GH-561 profile driven review of VariantConverters.

Author: steveloughran

parquet-benchmarks/run.sh

@@ -100,6 +100,9 @@ else
   "filter")
     BENCHMARK_REGEX="org.apache.parquet.benchmarks.FilteringBenchmarks"
     ;;
+  "variant")
+    BENCHMARK_REGEX="org.apache.parquet.benchmarks.Variant*"
+    ;;
   esac
 
   echo JMH command: java -jar ${SCRIPT_PATH}/target/parquet-benchmarks.jar $BENCHMARK_REGEX $JMH_OPTIONS

parquet-benchmarks/src/main/java/org/apache/parquet/benchmarks/VariantProjectionBenchmark.java

@@ -88,9 +88,9 @@
  *   ./parquet-benchmarks/run.sh all org.apache.parquet.benchmarks.VariantProjectionBenchmark \
  *       -wi 3 -i 5 -f 1  -foe true -rf json -rff target/results.json
  * </pre>
- * *
+ *
  */
-@Fork(0)
+@Fork(1)
 @State(Scope.Benchmark)
 @Warmup(iterations = 3)
 @Measurement(iterations = 5)
@@ -533,19 +533,14 @@ public RowRecord getCurrentRecord() {
    * schema and projected schemas are handled correctly.
    */
   private static final class MessageConverter extends GroupConverter {
-    private final int idIndex;
-    private final int categoryIndex;
-    private final int nestedIndex;
+
     private final PrimitiveConverter idConverter;
     private final PrimitiveConverter categoryConverter;
     private final RowVariantGroupConverter variantConverter;
     private long id;
     private int category;
 
     MessageConverter(MessageType schema, GroupType nestedGroup) {
-      idIndex = schema.getFieldIndex("id");
-      categoryIndex = schema.getFieldIndex("category");
-      nestedIndex = schema.getFieldIndex("nested");
       idConverter = new PrimitiveConverter() {
         @Override
         public void addLong(long value) {
@@ -563,10 +558,16 @@ public void addInt(int value) {
 
     @Override
     public Converter getConverter(int fieldIndex) {
-      if (fieldIndex == idIndex) return idConverter;
-      if (fieldIndex == categoryIndex) return categoryConverter;
-      if (fieldIndex == nestedIndex) return variantConverter;
-      throw new IllegalArgumentException("Unknown field index: " + fieldIndex);
+      switch (fieldIndex) {
+        case 0:
+          return idConverter;
+        case 1:
+          return categoryConverter;
+        case 2:
+          return variantConverter;
+        default:
+          throw new IllegalArgumentException("Unknown field index: " + fieldIndex);
+      }
     }
 
     @Override

parquet-variant/src/main/java/org/apache/parquet/variant/VariantBuilder.java

@@ -24,6 +24,7 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.Set;
+import org.apache.parquet.io.api.Binary;
 
 /**
  * Builder for creating Variant value and metadata.
@@ -109,7 +110,14 @@ public void appendEncodedValue(ByteBuffer value) {
    */
   public void appendString(String str) {
     onAppend();
-    byte[] data = str.getBytes(StandardCharsets.UTF_8);
+    writeUTF8bytes(str.getBytes(StandardCharsets.UTF_8));
+  }
+
+  /**
+   * Write bytes as a UTF8 string.
+   * @param data data to write; this is not modified.
+   */
+  private void writeUTF8bytes(final byte[] data) {
     boolean longStr = data.length > VariantUtil.MAX_SHORT_STR_SIZE;
     checkCapacity((longStr ? 1 + VariantUtil.U32_SIZE : 1) + data.length);
     if (longStr) {
@@ -125,6 +133,17 @@ public void appendString(String str) {
     writePos += data.length;
   }
 
+  /**
+   * Given a Binary, append it to the variant as a string.
+   * When unmarshalling from partially shredded types, this saves string creation.
+   * This is not for use by the VariantConverters.
+   * @param binary source data.
+   */
+  void appendAsString(Binary binary) {
+    onAppend();
+    writeUTF8bytes(binary.getBytesUnsafe());
+  }
+
   /**
    * Appends a null value to the Variant builder.
    */

parquet-variant/src/main/java/org/apache/parquet/variant/VariantConverters.java

@@ -233,14 +233,14 @@ static class PartiallyShreddedFieldsConverter extends GroupConverter {
     PartiallyShreddedFieldsConverter(GroupType fieldsType, ParentConverter<VariantBuilder> parent) {
       this.converters = new Converter[fieldsType.getFieldCount()];
       this.parent = parent;
+      ParentConverter<VariantObjectBuilder> newParent = converter -> converter.accept(objectBuilder);
 
       for (int index = 0; index < fieldsType.getFieldCount(); index += 1) {
         Type field = fieldsType.getType(index);
         Preconditions.checkArgument(!field.isPrimitive(), "Invalid field group: " + field);
 
         String name = field.getName();
         shreddedFieldNames.add(name);
-        ParentConverter<VariantObjectBuilder> newParent = converter -> converter.accept(objectBuilder);
         converters[index] = new FieldValueConverter(name, field.asGroupType(), newParent);
       }
     }
@@ -501,7 +501,7 @@ static class VariantStringConverter extends ShreddedScalarConverter {
 
     @Override
     public void addBinary(Binary value) {
-      parent.build(builder -> builder.appendString(value.toStringUsingUTF8()));
+      parent.build(builder -> builder.appendAsString(value));
     }
   }