[GitHub] [druid] gianm commented on a change in pull request #9731: ColumnCapabilities.hasMultipleValues refactor

[GitBox <gi...@apache.org>]

gianm commented on a change in pull request #9731:
URL: https://github.com/apache/druid/pull/9731#discussion_r420318956



##########
File path: processing/src/main/java/org/apache/druid/segment/DimensionIndexer.java
##########
@@ -127,6 +127,19 @@
    */
   EncodedKeyComponentType processRowValsToUnsortedEncodedKeyComponent(@Nullable Object dimValues, boolean reportParseExceptions);
 
+  /**
+   * This method will be called whenever a known dimension is absent in a row to allow an indexer

Review comment:
       1. This javadoc doesn't make a ton of sense to me. Specifically it's not clear to me what "known dimension is absent in a row" means or what "account for any missing rows" might refer to. I wonder if you could find an improved wording.
   2. IMO it'd be better to not have this method be `default`. It makes it too easy to forget to override it when necessary.

##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilities.java
##########
@@ -57,6 +50,21 @@ public boolean isTrue()
       return this == TRUE;
     }
 
+    public boolean isMaybeTrue()
+    {
+      return isTrue() || isUnknown();
+    }
+
+    public boolean isUnknown()
+    {
+      return this == UNKNOWN;
+    }
+
+    public Capable complete(boolean convertUnknownToTrue)

Review comment:
       The boolean here is tough for me to wrap my head around: when looking at method calls like `.complete(true)` and `.complete(false)`, it looks like it's describing whether or not things are complete. I think it would be better to use a two-valued enum, so the call would be `.makeComplete(UNKNOWN_TO_FALSE)` or `.makeComplete(UNKNOWN_TO_TRUE)`.

##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java
##########
@@ -31,23 +31,66 @@
  */
 public class ColumnCapabilitiesImpl implements ColumnCapabilities
 {
-  public static ColumnCapabilitiesImpl copyOf(final ColumnCapabilities other)
+  public static ColumnCapabilitiesImpl copyOf(@Nullable final ColumnCapabilities other)
   {
     final ColumnCapabilitiesImpl capabilities = new ColumnCapabilitiesImpl();
-    capabilities.merge(other);
-    capabilities.setFilterable(other.isFilterable());
-    capabilities.setIsComplete(other.isComplete());
+    if (other != null) {
+      capabilities.type = other.getType();
+      capabilities.dictionaryEncoded = other.isDictionaryEncoded();
+      capabilities.runLengthEncoded = other.isRunLengthEncoded();
+      capabilities.hasInvertedIndexes = other.hasBitmapIndexes();
+      capabilities.hasSpatialIndexes = other.hasSpatialIndexes();
+      capabilities.hasMultipleValues = other.hasMultipleValues();
+      capabilities.dictionaryValuesSorted = other.areDictionaryValuesSorted();
+      capabilities.dictionaryValuesUnique = other.areDictionaryValuesUnique();
+      capabilities.filterable = other.isFilterable();
+    }
     return capabilities;
   }
 
+  /**
+   * Used at indexing time to finalize all {@link ColumnCapabilities.Capable#UNKNOWN} values to
+   * {@link ColumnCapabilities.Capable#FALSE}, in order to present a snapshot of the state of the this column
+   */
+  @Nullable
+  public static ColumnCapabilitiesImpl complete(
+      @Nullable final ColumnCapabilities capabilities,
+      boolean convertUnknownToTrue
+  )
+  {
+    if (capabilities == null) {
+      return null;
+    }
+    ColumnCapabilitiesImpl copy = copyOf(capabilities);
+    copy.hasMultipleValues = copy.hasMultipleValues.complete(convertUnknownToTrue);
+    copy.dictionaryValuesSorted = copy.dictionaryValuesSorted.complete(convertUnknownToTrue);
+    copy.dictionaryValuesUnique = copy.dictionaryValuesUnique.complete(convertUnknownToTrue);
+    return copy;
+  }
+
+
+  /**
+   * Create a no frills, simple column with {@link ValueType} set and everything else false
+   */
+  public static ColumnCapabilitiesImpl createSimpleNumericColumn(ValueType valueType)

Review comment:
       `createSimpleNumericCapabilities`?

##########
File path: processing/src/main/java/org/apache/druid/segment/ColumnProcessors.java
##########
@@ -197,6 +197,6 @@
    */
   private static boolean mayBeMultiValue(@Nullable final ColumnCapabilities capabilities)
   {
-    return capabilities == null || !capabilities.isComplete() || capabilities.hasMultipleValues();
+    return capabilities == null || capabilities.hasMultipleValues().isMaybeTrue();

Review comment:
       Nice.

##########
File path: processing/src/main/java/org/apache/druid/segment/ColumnSelectorBitmapIndexSelector.java
##########
@@ -160,11 +160,11 @@ public void close() throws IOException
   public boolean hasMultipleValues(final String dimension)

Review comment:
       IMO, better if this returns a `Capable`. Then the caller can decide what to do with unknowns.

##########
File path: processing/src/main/java/org/apache/druid/segment/DimensionHandlerUtils.java
##########
@@ -60,7 +60,8 @@
                                   .setDictionaryEncoded(false)
                                   .setDictionaryValuesUnique(false)
                                   .setDictionaryValuesSorted(false)
-                                  .setHasBitmapIndexes(false);
+                                  .setHasBitmapIndexes(false)
+                                  .setHasMultipleValues(false);

Review comment:
       Why this change?
   
   As I understand it, the previous behavior is roughly equivalent to returning unknown for HMV. (Because it would return false, but complete is also false.)

##########
File path: processing/src/main/java/org/apache/druid/segment/incremental/IncrementalIndexStorageAdapter.java
##########
@@ -142,24 +141,8 @@ public Comparable getMaxValue(String column)
   @Override
   public ColumnCapabilities getColumnCapabilities(String column)
   {
-    // Different from index.getCapabilities because, in a way, IncrementalIndex's string-typed dimensions
-    // are always potentially multi-valued at query time. (Missing / null values for a row can potentially be
-    // represented by an empty array; see StringDimensionIndexer.IndexerDimensionSelector's getRow method.)
-    //
-    // We don't want to represent this as having-multiple-values in index.getCapabilities, because that's used
-    // at index-persisting time to determine if we need a multi-value column or not. However, that means we
-    // need to tweak the capabilities here in the StorageAdapter (a query-time construct), so at query time
-    // they appear multi-valued.
-
-    final ColumnCapabilities capabilitiesFromIndex = index.getCapabilities(column);
-    final IncrementalIndex.DimensionDesc dimensionDesc = index.getDimension(column);
-    if (dimensionDesc != null && dimensionDesc.getCapabilities().getType() == ValueType.STRING) {
-      final ColumnCapabilitiesImpl retVal = ColumnCapabilitiesImpl.copyOf(capabilitiesFromIndex);
-      retVal.setHasMultipleValues(true);
-      return retVal;
-    } else {
-      return capabilitiesFromIndex;
-    }
+    // snapshot the current state
+    return ColumnCapabilitiesImpl.complete(index.getCapabilities(column), false);

Review comment:
       Why `false` here instead of `true`?
   
   My understanding is that the HMV capability in the IncrementalIndex is now either going to be UNKNOWN (where it starts) or TRUE (if there are actually MVs). So UNKNOWN means we haven't seen any MV inputs yet.
   
   But for the reason mentioned in the comment you deleted (the behavior of `StringDimensionIndexer.IndexerDimensionSelector's getRow method`), can't we get empty arrays from dimension selectors when querying IncrementalIndexes, even if the capability is UNKNOWN? So shouldn't we treat an unknown here as `true` rather than `false`?
   
   I must be missing something, since the tests are passing, assuming we have tests for this…

##########
File path: processing/src/main/java/org/apache/druid/segment/column/ColumnCapabilitiesImpl.java
##########
@@ -31,23 +31,66 @@
  */
 public class ColumnCapabilitiesImpl implements ColumnCapabilities
 {
-  public static ColumnCapabilitiesImpl copyOf(final ColumnCapabilities other)
+  public static ColumnCapabilitiesImpl copyOf(@Nullable final ColumnCapabilities other)
   {
     final ColumnCapabilitiesImpl capabilities = new ColumnCapabilitiesImpl();
-    capabilities.merge(other);
-    capabilities.setFilterable(other.isFilterable());
-    capabilities.setIsComplete(other.isComplete());
+    if (other != null) {
+      capabilities.type = other.getType();
+      capabilities.dictionaryEncoded = other.isDictionaryEncoded();
+      capabilities.runLengthEncoded = other.isRunLengthEncoded();
+      capabilities.hasInvertedIndexes = other.hasBitmapIndexes();
+      capabilities.hasSpatialIndexes = other.hasSpatialIndexes();
+      capabilities.hasMultipleValues = other.hasMultipleValues();
+      capabilities.dictionaryValuesSorted = other.areDictionaryValuesSorted();
+      capabilities.dictionaryValuesUnique = other.areDictionaryValuesUnique();
+      capabilities.filterable = other.isFilterable();
+    }
     return capabilities;
   }
 
+  /**
+   * Used at indexing time to finalize all {@link ColumnCapabilities.Capable#UNKNOWN} values to

Review comment:
       Is this ever called with `convertUnknownToTrue == true`? If not, it could be named better.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org