[GitHub] [druid] clintropolis commented on a change in pull request #12127: Use ListFilteredVirtualColumn for left/fact table expression in join condition

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

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



##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/MultiValueStringOperatorConversions.java
##########
@@ -361,6 +363,17 @@ public DruidExpression toDruidExpression(
                 isAllowList()
             )
         );
+
+        if (plannerContext.getVirtualColumnRegistry() != null) {

Review comment:
       this feels worthy of a comment explaining what is going on

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidQuery.java
##########
@@ -161,7 +161,9 @@ public static DruidQuery fromPartialQuery(
   )
   {
     final RelDataType outputRowType = partialQuery.leafRel().getRowType();
-    final VirtualColumnRegistry virtualColumnRegistry = VirtualColumnRegistry.create(sourceRowSignature);
+    final VirtualColumnRegistry virtualColumnRegistry = plannerContext.getVirtualColumnRegistry() == null
+                                                        ? VirtualColumnRegistry.create(sourceRowSignature)
+                                                        : plannerContext.getVirtualColumnRegistry();

Review comment:
       this seems a bit confusing to me. Is the planner context virtual column registry only used for joins I guess, at least based on the changes in this PR? 
   
   A number of methods, particularly SQL conversion stuffs, accept a `VirtualColumnRegistry` as an argument, and also accept a `PlannerContext`. I guess at least this change probably means that those are the same thing and so that callers do not need to check the planner context, because if it set then it is also the argument.

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/DruidJoinQueryRel.java
##########
@@ -163,6 +163,9 @@ public DruidQuery toDruidQuery(final boolean finalizeAggregations)
 
     final Pair<String, RowSignature> prefixSignaturePair = computeJoinRowSignature(leftSignature, rightSignature);
 
+    VirtualColumnRegistry virtualColumnRegistry = VirtualColumnRegistry.create(prefixSignaturePair.rhs);
+    getPlannerContext().setVirtualColumnRegistry(virtualColumnRegistry);

Review comment:
       can there only be 1 `DruidJoinQueryRel` in a single query? (if not then i think the virtual column registry probably needs to be set somewhere else)

##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/PlannerContext.java
##########
@@ -392,4 +394,14 @@ public QueryMaker getQueryMaker()
   {
     return Preconditions.checkNotNull(queryMaker, "QueryMaker not available");
   }
+
+  public VirtualColumnRegistry getVirtualColumnRegistry()

Review comment:
       nit: `@Nullable` i think?
   
   I think this maybe deserves javadocs as well, since it is not nullable, and seems to have situational usage
   
   also, is this actually safe? the planner context seems to be tied to the lifetime of the planner, but the current virtual column registry is tied to the lifetime of a `PartialDruidQuery`. I guess i'm wondering, does this mean that the virtual column registry in the plannerContext potentially have virtual columns from earlier attempted plans? I guess each new `DruidJoinQueryRel` will create a new registry (but also is that safe?)

##########
File path: processing/src/main/java/org/apache/druid/query/JoinDataSource.java
##########
@@ -221,6 +222,16 @@ public boolean isConcrete()
     return false;
   }
 
+  public Set<String> getVirtualColumnCandidates()

Review comment:
       javadoc please




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