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:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
