[GitHub] [druid] clintropolis commented on a change in pull request #10767: Vectorized theta sketch aggregator + rework of VectorColumnProcessorFactory.

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

clintropolis commented on a change in pull request #10767:
URL: https://github.com/apache/druid/pull/10767#discussion_r560893111



##########
File path: processing/src/main/java/org/apache/druid/segment/VectorColumnProcessorFactory.java
##########
@@ -22,34 +22,59 @@
 import org.apache.druid.segment.column.ColumnCapabilities;
 import org.apache.druid.segment.vector.MultiValueDimensionVectorSelector;
 import org.apache.druid.segment.vector.SingleValueDimensionVectorSelector;
+import org.apache.druid.segment.vector.VectorObjectSelector;
 import org.apache.druid.segment.vector.VectorValueSelector;
 
 /**
  * Class that encapsulates knowledge about how to create vector column processors. Used by
- * {@link DimensionHandlerUtils#makeVectorProcessor}.
+ * {@link ColumnProcessors#makeVectorProcessor}.
  *
- * Unlike {@link ColumnProcessorFactory}, this interface does not have a "defaultType" method. The default type is
- * always implicitly STRING. It also does not have a "makeComplexProcessor" method; instead, complex-typed columns
- * are fed into "makeSingleValueDimensionProcessor". This behavior may change in the future to better align
- * with {@link ColumnProcessorFactory}.
+ * Column processors can be any type "T". The idea is that a ColumnProcessorFactory embodies the logic for wrapping
+ * and processing selectors of various types, and so enables nice code design, where type-dependent code is not
+ * sprinkled throughout.
+ *
+ * Unlike {@link ColumnProcessorFactory}, this interface does not have a "defaultType" method, because vector
+ * column types are always known, so it isn't necessary.
  *
  * @see ColumnProcessorFactory the non-vectorized version
  */
 public interface VectorColumnProcessorFactory<T>
 {
+  /**
+   * Called when {@link ColumnCapabilities#getType()} is STRING and the underlying column always has a single value
+   * per row.
+   */
   T makeSingleValueDimensionProcessor(
       ColumnCapabilities capabilities,
       SingleValueDimensionVectorSelector selector
   );
 
+  /**
+   * Called when {@link ColumnCapabilities#getType()} is STRING and the underlying column may have multiple values
+   * per row.
+   */
   T makeMultiValueDimensionProcessor(
       ColumnCapabilities capabilities,
       MultiValueDimensionVectorSelector selector
   );
 
+  /**
+   * Called when {@link ColumnCapabilities#getType()} is FLOAT.
+   */
   T makeFloatProcessor(ColumnCapabilities capabilities, VectorValueSelector selector);
 
+  /**
+   * Called when {@link ColumnCapabilities#getType()} is DOUBLE.
+   */
   T makeDoubleProcessor(ColumnCapabilities capabilities, VectorValueSelector selector);
 
+  /**
+   * Called when {@link ColumnCapabilities#getType()} is LONG.
+   */
   T makeLongProcessor(ColumnCapabilities capabilities, VectorValueSelector selector);
+
+  /**
+   * Called when {@link ColumnCapabilities#getType()} is COMPLEX.
+   */
+  T makeComplexProcessor(ColumnCapabilities capabilities, VectorObjectSelector selector);

Review comment:
       Since this isn't actually specific to complex columns, can this be called `makeObjectProcessor`? I have the same method signature except with that name added to this class in #10613, there to support using object selectors for string expression virtual columns which are not dictionary encoded.




----------------------------------------------------------------
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