[GitHub] [druid] jihoonson commented on a change in pull request #12241: rework sql expressions and virtual columns

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

jihoonson commented on a change in pull request #12241:
URL: https://github.com/apache/druid/pull/12241#discussion_r803059825



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/VirtualColumnRegistry.java
##########
@@ -169,15 +212,75 @@ public RowSignature getFullRowSignature()
                      .collect(Collectors.toList());
   }
 
-  private static class ExpressionWrapper
+  /**
+   * @deprecated use {@link #getOrCreateVirtualColumnForExpression(DruidExpression, ColumnType)} instead
+   */
+  @Deprecated
+  public VirtualColumn getOrCreateVirtualColumnForExpression(
+      PlannerContext plannerContext,
+      DruidExpression expression,
+      ColumnType valueType
+  )
+  {
+    final String name = getOrCreateVirtualColumnForExpression(expression, valueType);
+    return virtualColumnsByName.get(name).expression.toVirtualColumn(name, valueType, macroTable);
+  }
+
+  /**
+   * @deprecated use {@link #getOrCreateVirtualColumnForExpression(DruidExpression, RelDataType)} instead
+   */
+  @Deprecated
+  public VirtualColumn getOrCreateVirtualColumnForExpression(
+      PlannerContext plannerContext,
+      DruidExpression expression,
+      RelDataType dataType
+  )
   {
-    private final String expression;
-    private final ColumnType valueType;
+    return getOrCreateVirtualColumnForExpression(
+        plannerContext,
+        expression,
+        Calcites.getColumnTypeForRelDataType(dataType)
+    );
+  }
 
-    public ExpressionWrapper(String expression, ColumnType valueType)
+  /**
+   * @deprecated use {@link #getVirtualColumnByExpression(DruidExpression, RelDataType)} instead
+   */
+  @Deprecated
+  @Nullable
+  public VirtualColumn getVirtualColumnByExpression(String expression, RelDataType type)
+  {
+    final ColumnType columnType = Calcites.getColumnTypeForRelDataType(type);
+    ExpressionAndTypeHint wrapped = wrap(DruidExpression.fromExpression(expression), columnType);
+    return Optional.ofNullable(virtualColumnsByExpression.get(wrapped))
+                   .map(this::getVirtualColumn)
+                   .orElse(null);
+  }
+
+  private static ExpressionAndTypeHint wrap(DruidExpression expression, ColumnType typeHint)
+  {
+    return new ExpressionAndTypeHint(expression, typeHint);
+  }
+
+  private static class ExpressionAndTypeHint
+  {
+    private final DruidExpression expression;
+    private final ColumnType typeHint;

Review comment:
       nit: `typeHint` seems worth to document. Can you please add some javadoc for it? It's not a blocker and you can do it as a follow-up.

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/DruidExpression.java
##########
@@ -32,127 +32,303 @@
 import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
 
 import javax.annotation.Nullable;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
 import java.util.function.Function;
 
 /**
+ * todo(clint): rewrite javadocs
+ *
  * Represents two kinds of expression-like concepts that native Druid queries support:
  *
  * (1) SimpleExtractions, which are direct column access, possibly with an extractionFn
  * (2) native Druid expressions.
  */
 public class DruidExpression
 {
+  public enum NodeType
+  {
+    /**
+     * constant value
+     */
+    LITERAL,
+    /**
+     * Identifier for a direct physical or virtual column access (column name or virtual column name)
+     */
+    IDENTIFIER,
+    /**
+     * Standard native druid expression, which can compute a string that can be parsed into {@link Expr}, or used
+     * as an {@link ExpressionVirtualColumn}
+     */
+    EXPRESSION,
+    /**
+     * Expression backed by a specialized {@link VirtualColumn}, which might provide more optimized evaluation than
+     * is possible with the standard
+     */
+    SPECIALIZED
+  }
+
   // Must be sorted
   private static final char[] SAFE_CHARS = " ,._-;:(){}[]<>!@#$%^&*`~?/".toCharArray();
+  private static final VirtualColumnBuilder DEFAULT_VIRTUAL_COLUMN_BUILDER = new ExpressionVirtualColumnBuilder();

Review comment:
       Seems more like a function than a builder, but we can change the name later if needed.

##########
File path: sql/src/test/java/org/apache/druid/sql/calcite/CalciteMultiValueStringQueryTest.java
##########
@@ -1137,9 +1151,16 @@ public void testMultiValueListFilterComposed() throws Exception
                         .setContext(QUERY_CONTEXT_DEFAULT)
                         .build()
         ),
-        ImmutableList.of(
+        useDefault ? ImmutableList.of(
             new Object[]{0, 4L},
             new Object[]{1, 2L}
+        ) : ImmutableList.of(
+            // the fallback expression would actually produce 3 rows, 2 nulls, 2 0's, and 2 1s
+            // instead of 4 nulls and two 1's we get when using the 'native' list filtered virtual column
+            // this is because of slight differences between filter and the native
+            // selector, which treats a 0 length array as null instead of an empty array like is produced by filter

Review comment:
       This seems worth the release notes. Can you add some release note section in the PR description, so that we won't forget later?




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

To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org

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