optimize constant auto columns by clintropolis · Pull Request #14412 · apache/druid

clintropolis · 2023-06-13T02:56:37Z

Description

changes:

auto columns can now specialize the case when the column contains only constants, avoiding writing any real columns and just producing constant selectors and indexes. since this is backwards incompatible it is gated behind a flag on IndexSpec, optimizeJsonConstantColumns, which should be removed in Druid 28
auto columns no longer participate in generic 'null column' handling, this was a mistake to try to support and caused ingestion failures due to mismatched ColumnFormat, and is replaced by the new constant column functionality
fix bugs with auto columns which contain empty objects, empty arrays, or primitive types mixed with either of these empty constructs

Release note

TBD

This PR has:

changes: * auto columns can now specialize the case when the column contains only constants, avoiding writing any real columns and just producing constant selectors and indexes. since this is backwards incompatible it is gated behind a flag on IndexSpec, optimizeJsonConstantColumns, which should be removed in Druid 28 * auto columns no longer participate in generic 'null column' handling, this was a mistake to try to support and caused ingestion failures due to mismatched ColumnFormat, and is replaced by the new constant column functionality * fix bugs with auto columns which contain empty objects, empty arrays, or primitive types mixed with either of these empty constructs

processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java

processing/src/main/java/org/apache/druid/segment/nested/ConstantColumnAndIndexSupplier.java

processing/src/test/java/org/apache/druid/segment/nested/ConstantColumnSupplierTest.java

+      if (row != null) {
+        if (row instanceof List) {
+          Assert.assertArrayEquals(((List) row).toArray(), (Object[]) valueSelector.getObject());
+          if (expectedType.getSingleType() != null) {


processing/src/test/java/org/apache/druid/segment/nested/ConstantColumnSupplierTest.java

+  )
+      throws IOException
+  {
+    SegmentWriteOutMediumFactory writeOutMediumFactory = TmpFileSegmentWriteOutMediumFactory.instance();


imply-cheddar · 2023-06-14T04:19:10Z

...sing/src/test/java/org/apache/druid/segment/serde/NestedCommonFormatColumnPartSerdeTest.java

+    NestedCommonFormatColumnPartSerde partSerde = NestedCommonFormatColumnPartSerde.serializerBuilder()
+                                                                                   .isConstant(false)
+                                                                                   .isVariantType(false)
+                                                                                   .withByteOrder(ByteOrder.nativeOrder())
+                                                                                   .withHasNulls(true)
+                                                                                   .withLogicalType(ColumnType.LONG)
+                                                                                   .build();


I think that CodeQL has a point here, is this test doing something?

processing/src/test/java/org/apache/druid/segment/nested/ConstantColumnSupplierTest.java

+  DefaultBitmapResultFactory resultFactory = new DefaultBitmapResultFactory(bitmapSerdeFactory.getBitmapFactory());
+
+  public ConstantColumnSupplierTest(
+      @SuppressWarnings("unused") String name,


imply-cheddar · 2023-06-14T03:49:23Z

processing/src/main/java/org/apache/druid/segment/nested/ConstantColumn.java

+      } else {
+        nulls = null;
+      }
+      switch (logicalType.getType()) {


Can you add a default case that just falls through and have a comment that explains why it's good to fall through?

…pecified

imply-cheddar

I think we can do better than using a Jackson-serialized form of the object as our constant identifier and would hope that we would do that before persisting any constant columns. So, I think that for this PR we either separate out the "store constant columns" into a different PR or update this PR to store the constants as a type and relevant dictionaryId.

imply-cheddar · 2023-06-14T03:09:24Z

processing/src/main/java/org/apache/druid/query/metadata/SegmentAnalyzer.java

+      if (capabilities != null) {
+        bob.hasMultipleValues(capabilities.hasMultipleValues().isTrue())
+           .hasNulls(capabilities.hasNulls().isMaybeTrue());
+      }


Does this need to be inside of the try? I don't think it does, but I'm wondering if I'm missing something.

imply-cheddar · 2023-06-14T03:22:34Z

processing/src/main/java/org/apache/druid/segment/AutoTypeColumnIndexer.java

-      FieldIndexer rootField = fieldIndexers.get(NestedPathFinder.JSON_PATH_ROOT);
-      ColumnType singleType = rootField.getTypes().getSingleType();
-      return singleType == null ? ColumnType.NESTED_DATA : singleType;
+    if (!hasNestedData) {


Nit: make this positive instead of negative.

imply-cheddar · 2023-06-14T03:23:39Z

processing/src/main/java/org/apache/druid/segment/AutoTypeColumnIndexer.java

+      if (fieldIndexers.isEmpty()) {
+        // we didn't see anything, so we can be anything, so why not a string?
+        return ColumnType.STRING;
+      }


I would've expected this case to be isConstant = true and constantValue = null, why not check for those states instead?

imply-cheddar · 2023-06-14T03:44:46Z

processing/src/main/java/org/apache/druid/segment/QueryableIndexIndexableAdapter.java

+      if (column instanceof ConstantColumn) {
+        return new NestedColumnMergable(
+            new SortedValueDictionary(
+                column.getStringDictionary(),
+                column.getLongDictionary(),
+                column.getDoubleDictionary(),
+                column.getArrayDictionary(),
+                column
+            ),
+            column.getFieldTypeInfo(),
+            ColumnType.NESTED_DATA.equals(column.getLogicalType()),
+            true,
+            ((ConstantColumn) column).getConstantValue()
+        );
+      }


Why not move this check to be before the if( col instanceof NestedCommonFormatColumn) check. It doesn't seem to be meaningful that this is nested?

imply-cheddar · 2023-06-14T03:49:23Z

processing/src/main/java/org/apache/druid/segment/nested/ConstantColumn.java

+      } else {
+        nulls = null;
+      }
+      switch (logicalType.getType()) {


Can you add a default case that just falls through and have a comment that explains why it's good to fall through?

imply-cheddar · 2023-06-14T03:59:57Z

processing/src/main/java/org/apache/druid/segment/nested/ConstantColumnAndIndexSupplier.java

+        throw new RE(ex, "Failed to deserialize V%s column [%s].", version, columnName);
+      }
+    } else {
+      throw new RE("Unknown version " + version);


interpolate with []?

imply-cheddar · 2023-06-14T04:04:13Z

processing/src/main/java/org/apache/druid/segment/nested/ConstantColumnAndIndexSupplier.java

+        final Object constantValue = NestedDataComplexTypeSerde.OBJECT_MAPPER.readValue(
+            IndexMerger.SERIALIZER_UTILS.readBytes(bb, valueLength),
+            Object.class
+        );


We can do better than this. We know the type, let's persist the type and persist it as the lookup into whatever the relevant dictionary is. Given the type and the global dictionary id, we should be able to quite simply deserialize the thing...

imply-cheddar · 2023-06-14T04:07:15Z

processing/src/main/java/org/apache/druid/segment/nested/ConstantColumnAndIndexSupplier.java

+    this.matchBitmap = bitmapSerdeFactory.getBitmapFactory().complement(
+        bitmapSerdeFactory.getBitmapFactory().makeEmptyImmutableBitmap(),
+        numRows
+    );


Remind me of the lifecycle of the ConstantColumnAndIndexSupplier, I'm worried that it exists on-heap on segment-load, where I don't think it's good to pay the overhead of this on-heap object. It would be better to just build it on-demand at query time (it's not really expensive to build).

imply-cheddar · 2023-06-14T04:12:15Z

processing/src/main/java/org/apache/druid/segment/nested/NestedCommonFormatColumn.java

-    FieldTypeInfo.MutableTypeSet rootOnlyType = new FieldTypeInfo.MutableTypeSet().add(getLogicalType());
    SortedMap<String, FieldTypeInfo.MutableTypeSet> fields = new TreeMap<>();
-    fields.put(NestedPathFinder.JSON_PATH_ROOT, rootOnlyType);
+    if (!getLogicalType().equals(ColumnType.NESTED_DATA)) {


nit: invert it so that there's 0 chance of an NPE. I.e. ColumnType.NESTED_DATA.equals(getLogicalType()) cannot NPE on this line

imply-cheddar · 2023-06-14T04:19:10Z

...sing/src/test/java/org/apache/druid/segment/serde/NestedCommonFormatColumnPartSerdeTest.java

+    NestedCommonFormatColumnPartSerde partSerde = NestedCommonFormatColumnPartSerde.serializerBuilder()
+                                                                                   .isConstant(false)
+                                                                                   .isVariantType(false)
+                                                                                   .withByteOrder(ByteOrder.nativeOrder())
+                                                                                   .withHasNulls(true)
+                                                                                   .withLogicalType(ColumnType.LONG)
+                                                                                   .build();


I think that CodeQL has a point here, is this test doing something?

clintropolis added Bug Area - Segment Format and Ser/De labels Jun 13, 2023

clintropolis force-pushed the auto-constant-columns-and-bug-fixes branch from 7060c71 to 8f3d8df Compare June 13, 2023 03:02

github-advanced-security bot found potential problems Jun 13, 2023

View reviewed changes

clintropolis added 2 commits June 13, 2023 16:48

fixes, add flag for constant column

eb17dce

fix for old variant columns which might have incorrect logical type s…

c5ec3e1

…pecified

imply-cheddar suggested changes Jun 14, 2023

View reviewed changes

clintropolis mentioned this pull request Jun 14, 2023

auto columns fixes #14422

Merged

10 tasks

clintropolis closed this Jun 14, 2023

Conversation

clintropolis commented Jun 13, 2023

Description

Release note

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check warning

Check notice

Check notice

Choose a reason for hiding this comment

Uh oh!

Check notice

Check warning

Choose a reason for hiding this comment

Uh oh!

imply-cheddar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants