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

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

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



##########
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:
       Sure, although I'll keep the javadoc the same for now, since as of this patch it's still only called when the type is COMPLEX. It seems that one of us will need to rework implementations a bit based on which patch gets merged first.




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