Support Union in decoupled mode #15870

kgyrtkirk · 2024-02-08T18:14:10Z

to support Union some further changes were needed - as the original DruidQueryGenerator didn’t supported Dag -s.
after making the changes for that it was relatively easy to add Union
added a test to ensure that unused NotYetSupported modes are removed
moved DruidQueryGenerator to a separate package (I think in the future related classes may come here - which could give better overview)

* move baseruleset into a separate program * it doesn't interfere with the conversion rules * there will be less states in the planner during the conversion * enable scan&sort style queries to accept column reordings * add Window related conversion/etc to enable it in decoupled mode * added `DecoupledTestConfig` to specify per-testcase customization * currently its being used to describe why the native query is not checked

…ompositeTrait

This reverts commit 0c132df.

This reverts commit 6e6c752.

…ndow2

sql/src/main/java/org/apache/druid/sql/calcite/rel/logical/DruidValues.java

gianm · 2024-02-15T15:25:12Z

sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java

@@ -262,6 +262,7 @@ private Program buildBaseRuleSetProgram(PlannerContext plannerContext)
    builder.addMatchLimit(CalciteRulesManager.HEP_DEFAULT_MATCH_LIMIT);
    builder.addGroupBegin();
    builder.addRuleCollection(baseRuleSet(plannerContext));
+    builder.addRuleInstance(CoreRules.UNION_MERGE);


I feel the name of the method is no longer accurate (it's going beyond baseRuleSet). It appears this is only used in decoupled mode-- is that correct? If so some comments about that, or reflecting it in the name, would be useful.

Haven't read the rest of the patch as yet, so this is just a single comment rather than a review.

I wanted to separate basic optimiazation rules from the conversion rules - it will be usefull to be able to enable/tweak rules without altering the other planning rules.

It kinda already made one things easier: I was able to just throw in UNION_MERGE - without having to worry about disturbing existing plan differences.

there will be some more deeper tweaking will be needed as AGGREGATE_REMOVE and AGGREGATE_CASE_TO_FILTER doesn't play nicely together - which induces some plan differences

I can rename it to buildDecoupledLogicalOptimizationProgram - does that sound better?

rohangarg

Thanks a lot for the changes @kgyrtkirk :)
I have left a few comments, please let me know your thoughts!

sql/src/main/java/org/apache/druid/sql/calcite/planner/CalciteRulesManager.java

sql/src/main/java/org/apache/druid/sql/calcite/planner/querygen/InputDescProducer.java

sql/src/main/java/org/apache/druid/sql/calcite/rel/logical/DruidTableScan.java

sql/src/main/java/org/apache/druid/sql/calcite/rel/logical/DruidUnion.java

sql/src/main/java/org/apache/druid/sql/calcite/rule/logical/DruidTableScanRule.java

sql/src/test/java/org/apache/druid/sql/calcite/NotYetSupportedUsageTest.java

rohangarg · 2024-02-19T13:03:47Z

sql/src/main/java/org/apache/druid/sql/calcite/planner/querygen/DruidQueryGenerator.java

+    }
+    if (newInputs.size() == 1) {
+      Vertex inputVertex = newInputs.get(0);
+      Vertex newVertex = inputVertex.mergeNode(node, isRoot);


instead of mergeNode, does this seem more like a addParent/ buildParent?
Also, the method name can contain a maybe too since it can return null as well.

I've used Optional instead maybe - this made me remove some apidoc as well ; as now the returntype tells the story (instead of me in an apidoc)

Since we have a Vertex here the use of the keyword parent might be misleading.

I've renamed it to extendWith ; new method signature:

Optional<Vertex> extendWith(RelNode parentNode, boolean isRoot)

sql/src/main/java/org/apache/druid/sql/calcite/planner/querygen/DruidQueryGenerator.java

rohangarg · 2024-02-19T13:07:50Z

sql/src/main/java/org/apache/druid/sql/calcite/planner/querygen/DruidQueryGenerator.java

+      @Nullable
+      public PartialDruidQuery mergeNode1(RelNode node, boolean isRoot)
+      {
+        if (accepts(node, Stage.WHERE_FILTER, Filter.class)) {


In future, should we move this code to respective DruidLogicalNode themselves? That interface can have an accept method and the input to this method could be a DruidLogicalNode instead of a RelNode?

Right now I think PartialDruidQuery is a way to build the Query - and I believe there might be other ways to build queries in the future - to keep that separation natural: I think the builder should hide away the conversion logic.

From another viewpoint: if there is another way to build Query (not thru PartialDruidQuery) ; then the nodes it uses why contain PartialDruidQuery related stuff?

Maybe the common denominator would be to have a target build related node classes like PDQFilter / PDQProject ... that might be another approach we could take later on....

rohangarg

LGTM 👍
Request to address the comment about test runtime before merge

rohangarg · 2024-02-21T08:52:58Z

sql/src/test/java/org/apache/druid/sql/calcite/NotYetSupportedUsageTest.java

+  @Test
+  public void ensureAllModesUsed()
+  {
+    Set<Method> methodsAnnotatedWith = new Reflections("org.apache", new MethodAnnotationsScanner())


how much time does this test take? if it takes long enough, might be worth changing this prefix to org.apache.druid.sql

it took 1.595s on the CI

on my system:

1.3s without changes

0.45s with org.apache.druid.sql

experimented a but with changing scan configuration - but it just made it more complicated without much benefit... changed it to org.apache.druid.sql :D

rohangarg · 2024-02-21T15:55:17Z

Merged since the test failure was unrelated (in Nested Column ITs)

kgyrtkirk added 30 commits January 31, 2024 14:37

no more need for workaround that getTrait(T) sometimes returns RelC…

37d11f3

…ompositeTrait

lets not do this now

ce3bd03

describe stuff

8112d1d

add

5a1b740

add test to remove modes

274fbfa

some stuff

443058b

add fixme

0ba43ac

some unrelated

0c132df

Revert "some unrelated"

aec32da

This reverts commit 0c132df.

some stuff

d1b2dad

a different approach

2dd63ab

remove stuff

a8e04db

cleanup

7657e7d

temp-crap

9775c1a

talk

6e6c752

Revert "talk"

407d4f3

This reverts commit 6e6c752.

cleanup

c9dbe26

cleanup

ef01196

fix/etc union

1ebe421

updates

a83dd8d

cleanup

7a94653

move org.reflections to depManagement

4f78e85

checkstyle

6e9e5ee

cleanup

3bc9326

merge 2 enum entries

7ff59fc

Merge remote-tracking branch 'apache/master' into decouple-support-wi…

fd858b5

…ndow2

Merge branch 'decouple-support-window2' into decouple-support-union

df615e0

xinputprod

cab8b8f

partially undo

646559e

kgyrtkirk added 3 commits February 8, 2024 09:29

cleanup

6f678b4

add some apidoc

a18726b

raise class security a bit

797e806

github-actions bot added Area - Querying Area - Dependencies labels Feb 8, 2024

github-advanced-security bot found potential problems Feb 8, 2024

View reviewed changes

sql/src/main/java/org/apache/druid/sql/calcite/rel/logical/DruidValues.java Dismissed Show dismissed Hide dismissed

kgyrtkirk added 3 commits February 9, 2024 08:22

fix err

7bf90e5

fix bug

b0d5f9e

remove debug on by default

63630e3

kgyrtkirk marked this pull request as ready for review February 9, 2024 18:55

remove processUnion method

4600378

github-advanced-security bot found potential problems Feb 9, 2024

View reviewed changes

sql/src/main/java/org/apache/druid/sql/calcite/rel/logical/DruidValues.java Dismissed Show dismissed Hide dismissed

kgyrtkirk added 3 commits February 14, 2024 11:00

cleanup

84048a2

add test

c0307fa

enable test for CalciteUnionQueryTest

e3e9379

imply-cheddar approved these changes Feb 14, 2024

View reviewed changes

gianm reviewed Feb 15, 2024

View reviewed changes

rename method

87c345b

rohangarg reviewed Feb 19, 2024

View reviewed changes

kgyrtkirk added 5 commits February 19, 2024 15:45

change message

87f801c

update

a7e04e2

cleanup

7770e37

fix

3d5dcd7

fix java8

a7a1358

rohangarg approved these changes Feb 21, 2024

View reviewed changes

kgyrtkirk added 2 commits February 21, 2024 11:34

tries with reflections

65f2d1a

restrict to org.apache.druid.sql

98adcd0

rohangarg merged commit bcce080 into apache:master Feb 21, 2024
82 of 83 checks passed

adarshsanjeev added this to the 30.0.0 milestone May 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Union in decoupled mode #15870

Support Union in decoupled mode #15870

kgyrtkirk commented Feb 8, 2024 •

edited

Loading

gianm Feb 15, 2024

kgyrtkirk Feb 16, 2024

rohangarg left a comment

rohangarg Feb 19, 2024

kgyrtkirk Feb 20, 2024

rohangarg Feb 19, 2024

kgyrtkirk Feb 19, 2024

rohangarg left a comment

rohangarg Feb 21, 2024

kgyrtkirk Feb 21, 2024

rohangarg commented Feb 21, 2024

Support Union in decoupled mode #15870

Support Union in decoupled mode #15870

Conversation

kgyrtkirk commented Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohangarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohangarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohangarg commented Feb 21, 2024

kgyrtkirk commented Feb 8, 2024 •

edited

Loading