[GitHub] [druid] suneet-s commented on a change in pull request #10401: vectorized expressions and expression virtual columns

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

suneet-s commented on a change in pull request #10401:
URL: https://github.com/apache/druid/pull/10401#discussion_r492499110



##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -49,6 +51,16 @@
    */
   String name();
 
+  default boolean canVectorize(Expr.InputBindingTypes inputTypes, Expr lambda, List<Expr> args)
+  {
+    return false;
+  }
+
+  default <T> ExprVectorProcessor<T> asVectorProcessor(Expr.VectorInputBindingTypes inputTypes, Expr lambda, List<Expr> args)

Review comment:
       javadocs for these functions please.

##########
File path: core/src/main/java/org/apache/druid/math/expr/ApplyFunction.java
##########
@@ -49,6 +51,16 @@
    */
   String name();
 
+  default boolean canVectorize(Expr.InputBindingTypes inputTypes, Expr lambda, List<Expr> args)
+  {
+    return false;
+  }
+
+  default <T> VectorExprProcessor<T> asVectorProcessor(Expr.VectorInputBindingTypes inputTypes, Expr lambda, List<Expr> args)

Review comment:
       javadocs for both of these functions please

##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java
##########
@@ -5912,9 +5912,6 @@ public void testExpressionFilteringAndGrouping() throws Exception
   @Test
   public void testExpressionFilteringAndGroupingUsingCastToLong() throws Exception
   {
-    // Cannot vectorize due to virtual columns.

Review comment:
       🎉 so much more vectorization!

##########
File path: core/src/main/java/org/apache/druid/math/expr/Expr.java
##########
@@ -148,6 +171,47 @@ default ExprType getOutputType(InputBindingTypes inputTypes)
   {
     @Nullable
     ExprType getType(String name);
+
+    default boolean areNumeric(List<Expr> args)
+    {
+      boolean numeric = args.size() > 0;

Review comment:
       Why is an empty list non numeric?
   
   Maybe add javadocs to clarify this behavior.

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/VirtualColumnRegistry.java
##########
@@ -136,9 +137,11 @@ public RowSignature getFullRowSignature()
     final RowSignature.Builder builder =
         RowSignature.builder().addAll(baseRowSignature);
 
+    ColumnInspector baseRowsInspector = builder.build().asColumnInspector();

Review comment:
       Does this mean a virtual column can't reference another virtual column? since the ColumnInspector is built with just he `baseRowSignature`?




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