[GitHub] [incubator-druid] gianm commented on a change in pull request #7769: SQL: Allow select-sort-project query shapes.

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

gianm commented on a change in pull request #7769: SQL: Allow select-sort-project query shapes.
URL: https://github.com/apache/incubator-druid/pull/7769#discussion_r287974126
 
 

 ##########
 File path: sql/src/main/java/org/apache/druid/sql/calcite/rel/VirtualColumnRegistry.java
 ##########
 @@ -32,52 +32,38 @@
 import java.util.TreeSet;
 
 /**
- * Wraps a {@link RowSignature} and provides facilities to re-use {@link VirtualColumn} definitions for dimensions,
- * filters, and filtered aggregators while constructing a {@link DruidQuery}
+ * Provides facilities to create and re-use {@link VirtualColumn} definitions for dimensions, filters, and filtered
+ * aggregators while constructing a {@link DruidQuery}.
  */
-public class DruidQuerySignature
+public class VirtualColumnRegistry
 
 Review comment:
   The point in this PR where I started splitting DruidQuerySignature into RowSignature + VirtualColumnRegistry was when writing this code:
   
   ```java
         this.sorting = Preconditions.checkNotNull(
             computeSorting(
                 partialQuery,
                 plannerContext,
                 computeOutputRowSignature(),
                 // When sorting follows grouping, virtual columns cannot be used
                 partialQuery.getAggregate() != null ? null : virtualColumnRegistry
             )
         );
   ```
   
   To get the proper DruidQuerySignature without the split would have meant doing:
   
   ```java
         RowSignature signature = computeOutputRowSignature();
         this.sorting = Preconditions.checkNotNull(
             computeSorting(
                 partialQuery,
                 plannerContext,
                 // When sorting follows grouping, virtual columns cannot be used
                 partialQuery.getAggregate() != null
                 ? new DruidQuerySignature(this.grouping.getOutputRowSignature()).asAggregateSignature(this.grouping.getOutputRowSignature())
                 : notSureWhatGoesHere
             )
         );
   ```
   
   `notSureWhatGoesHere` reflects this situation:
   
   - At this point, the sort might be after a select or a select + project.
   - If the sort is after a simple select, it should be `sourceQuerySignature`.
   - If the sort is after a select + project, then the sort refers to the project (i.e. the `selectProjection`) which changed the row signature, and might have created its own virtual columns. Passing in `sourceQuerySignature` is wrong, since it's the wrong starting signature (it's pre-select-project). But passing in a new DruidQuerySignature is wrong too, since it will 'forget' about virtual columns that might have been used by the selectProjection.
   
   The solution seems to be to split it, so we can pass in `computeOutputRowSignature()`, the new post-selectProjection signature, along with the original `virtualColumnRegistry`.
   
   The change also made `computeHavingFilter` nicer, since it would take a RowSignature instead of DruidQuerySignature. (That was the only place where asAggregateSignature was used.)
   
   I thought it also made it clearer in various places what was going on, if a bit more verbose.

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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org