imply-cheddar commented on code in PR #17039:
URL: https://github.com/apache/druid/pull/17039#discussion_r1759008353
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/querygen/DruidQueryGenerator.java:
##########
@@ -58,32 +60,64 @@ public DruidQueryGenerator(PlannerContext plannerContext,
DruidLogicalNode relRo
this.vertexFactory = new PDQVertexFactory(plannerContext, rexBuilder);
}
+ static class DruidNodeStack extends Stack<DruidLogicalNode>
Review Comment:
Another option instead of maintaining another parallel Stack would be to add
a mutable `operandIndex` field to `DruidLogicalNode` and walk the logical nodes
assigning ids once before doing the translation.
I tend to think that mutating the object to have an id and then referencing
that id is a bit less error prone than the parallel Stack approach, is there a
reason that it cannot work?
##########
sql/src/test/quidem/org.apache.druid.sql.calcite.DecoupledPlanningCalciteQueryTest/testGroupByWithLiteralInSubqueryGrouping@NullHandling=sql.iq:
##########
@@ -30,8 +30,8 @@ SELECT
+-------+----+
| t1 | t2 |
+-------+----+
-| dummy | |
| dummy | b |
+| dummy | |
Review Comment:
I'm surprised by this. `null` tends to sort first rather than last. This
makes me think that SQL compat mode has inverted null comparisons? More than
that, I really don't understand why your changes here would've caused this?
##########
sql/src/test/java/org/apache/druid/sql/calcite/DecoupledTestConfig.java:
##########
@@ -86,10 +84,8 @@ enum QuidemTestCaseReason
* New scans / etc.
*/
DEFINETLY_WORSE_PLAN,
- /**
- * A new {@link FinalizingFieldAccessPostAggregator} appeared in the plan.
- */
- FINALIZING_FIELD_ACCESS;
+ EQUIV_PLAN_EXTRA_COLUMNS,
+ EQUIV_PLAN_CAST_MATERIALIZED_EARLIER;
Review Comment:
I think I know what `EXTRA_COLUMNS` means, but `CAST_MATERIALIZED_EARLIER`
I'm unclear on. Can you add the comments for these like you have on the others?
##########
sql/src/main/java/org/apache/druid/sql/calcite/planner/querygen/DruidQueryGenerator.java:
##########
@@ -133,6 +168,42 @@ private interface Vertex
SourceDesc unwrapSourceDesc();
}
+ enum JoinSupportTweaks
+ {
+ NONE,
+ LEFT,
+ RIGHT;
+
+ static JoinSupportTweaks analyze(DruidNodeStack stack)
+ {
+ if (stack.size() < 2) {
+ return NONE;
+ }
+ DruidLogicalNode possibleJoin = stack.get(stack.size() - 2);
+ if (!(possibleJoin instanceof DruidJoin)) {
+ return NONE;
+ }
+ if (stack.operandIndexStack.get(stack.size() - 1) == 1) {
+ return RIGHT;
+ } else {
+ return LEFT;
+ }
+ }
Review Comment:
I find this method to be a bit obtuse. I *think* it's trying to figure out
what to put on the right hand side (prioritizing queries on the right?). But,
`== 1` in the final if doesn't really carry semantic meaning for me. It would
be nice if you could either adjust the code to be a bit more descriptive in
what it's doing or add comments that explain the magic numbers.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
