Unnest now works on MSQ #14886

somu-imply · 2023-08-21T16:05:01Z

This entails:

Removing the enableUnnest flag and additional machinery
Updating the datasource plan and frame processors to support unnest
Adding support in MSQ for UnnestDataSource and FilteredDataSource
CalciteArrayTest now has a MSQ test component
Additional tests for Unnest on MSQ
Removing engine feature of unnest alongside (1)

This PR has:

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

…d can have non empty side channels

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

...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java

cryptoe

Overall LGTM. Left some comments.

sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidRules.java

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

cryptoe · 2023-09-04T13:46:46Z

...re/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/BaseLeafFrameProcessor.java

+    // An UnnestDataSource or FilteredDataSource can have a join as a base
+    // In such a case a side channel is expected to be there
+    if (!(dataSource instanceof JoinDataSource
+          || dataSource instanceof UnnestDataSource


Unnest seems to have 2 data sources FilteredDS and UnnestDS. Could you please help me understand a bit more here.
Seems like the filter could have easily been pushed to the unnest data source. Is there are reason not to ?

nit: If we are keeping it as is, then there should be a better pattern than doing an instanceof for all.

Question:
What happens if we have a simple unnest query like -

SELECT * FROM tab1,UNNEST(col1)

What is expected to be in the side channel then? Seems like that they should be empty in this case

Yes, a Filtered data source is a data source with a filter. It was introduced for an issue where unnesting over a data source with a selector filter was planned as a QueryDataSource instead of a table data source. A FilteredDS is basically a base data source with a filter. It pushes a filter to the data source and not requiring a query data source in between does not need results to be materialized to the broker

In that case, should we just check that the delegate of the FilteredDataSource is not a JoinDataSource or an UnnestDataSource, instead of doing the check on the FilteredDataSource as a whole?
This will help if the FilteredDataSource evolves to have a delegate that is something other than the unnest data source.

Now that I think, we just need to check if the FilteredDataSource or UnnestDataSource has a Join as its base

This should be recursive which we already do. It would be good if we could cleanly merge this with the existing code path, that recurses on the data source's children, though not necessary.

extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQSelectTest.java

…red data source

LakshSingla · 2023-09-05T20:46:52Z

...re/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/BaseLeafFrameProcessor.java

+    // An UnnestDataSource or FilteredDataSource can have a join as a base
+    // In such a case a side channel is expected to be there
+    if (!(dataSource instanceof JoinDataSource
+          || dataSource instanceof UnnestDataSource


nit: If we are keeping it as is, then there should be a better pattern than doing an instanceof for all.

LakshSingla · 2023-09-05T20:48:37Z

...re/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/BaseLeafFrameProcessor.java

+    // An UnnestDataSource or FilteredDataSource can have a join as a base
+    // In such a case a side channel is expected to be there
+    if (!(dataSource instanceof JoinDataSource
+          || dataSource instanceof UnnestDataSource


Question:
What happens if we have a simple unnest query like -

SELECT * FROM tab1,UNNEST(col1)

What is expected to be in the side channel then? Seems like that they should be empty in this case

LakshSingla · 2023-09-05T20:49:09Z

...re/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/BaseLeafFrameProcessor.java

@@ -107,7 +113,8 @@ private static Pair<List<ReadableFrameChannel>, BroadcastJoinHelper> makeInputCh
      inputChannels.add(baseInput.getChannel());
    }

-    if (dataSource instanceof JoinDataSource) {
+
+    if (dataSource instanceof JoinDataSource || dataSource instanceof UnnestDataSource || dataSource instanceof FilteredDataSource) {


Seeing the same pattern as above, it should be a good refactor candidate.

… That way Unnest can be turned on/off for individual engine such as MSQ if needed

…engines. That way Unnest can be turned on/off for individual engine such as MSQ if needed" This reverts commit d942772.

LakshSingla · 2023-09-11T18:28:14Z

...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java

+          broadcast
+      );
+    } else if (dataSource instanceof UnnestDataSource) {
+      checkQuerySegmentSpecIsEternity(dataSource, querySegmentSpec);


Re-confirm if this check is required here. I think it should be removed unless UNNEST on a table cannot have a time filter.

Were you able to confirm if the check is required?

I have asked around, I'll update it once I get back the comments. I have removed it as the base data source types should take care of it, but will add it back otherwise

Any updates on this? I wanted to get a final check on this before the approval

LakshSingla · 2023-09-11T18:29:56Z

...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java

@@ -137,6 +139,30 @@ public static DataSourcePlan forDataSource(
    } else if (dataSource instanceof LookupDataSource) {
      checkQuerySegmentSpecIsEternity(dataSource, querySegmentSpec);
      return forLookup((LookupDataSource) dataSource, broadcast);
+    } else if (dataSource instanceof FilteredDataSource) {
+      checkQuerySegmentSpecIsEternity(dataSource, querySegmentSpec);


See the other comment about the check. Perhaps this should also be removed since we would plan the base data source, and there would be individual checks there.
See the plan for broadcast joins - The base data source doesn't enforce the eternity check, however, it is required on the clauses. I think something of that sort should be applicable in Filtered and Unnest data sources (please confirm it though)

…ide engines

somu-imply · 2023-09-15T16:53:42Z

@LakshSingla @cryptoe I have addressed the comments, added the refactoring, and also removed the context level feature to push it into individual engines. Please re-review

LakshSingla

Last minute review comments, overall LGTM.

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

LakshSingla · 2023-09-15T17:23:23Z

sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidRules.java

@@ -123,6 +124,7 @@ public static List<RelOptRule> rules(PlannerContext plannerContext)
      retVal.add(DruidFilterUnnestRule.DruidProjectOnUnnestRule.instance());
    }

+


nit: Unnecessary

Suggested change

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

cryptoe

Left my final review. Please let me know once those are addressed. Will approve and merge.

...re/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/BaseLeafFrameProcessor.java

extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQReplaceTest.java

cryptoe · 2023-09-21T01:20:39Z

...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java

+        querySegmentSpec,
+        null,
+        maxWorkerCount,
+        Math.max(minStageNumber, subQueryDefBuilder.getNextStageNumber()),


Wont this line add a new stage ?

I think this should be fine as it won't be adding a new stage, however, this is effective Math.max(minStageNumber, 0) since the builder here doesn't contain any stages. We can simplify it as below:

Suggested change

Math.max(minStageNumber, subQueryDefBuilder.getNextStageNumber()),

minStageNumber,

Also, thinking about it, we probably didn't need to create a separate subqueryDefBuilder since we aren't adding a new stage anyway, we can pass in the same one to the recursive call which might be causing this confusion. Since there are a few changes pending, let's clean this up as well.

LakshSingla · 2023-09-21T05:16:18Z

From a convo with @somu-imply, it seems like queries with a time filter SELECT * FROM foo, unnest(col) WHERE __time >= t1 and __time < t2 doesn't prune over the segments it has to iterate. It will iterate over all of the segments, and then apply the filter on top of it.
If this is correct, we should change the query segment spec to break down the where clause in the filter and the segment spec. However, I am confused if it's the same behavior in native queries as well.

LakshSingla · 2023-09-21T05:36:49Z

Seems like the above is a problem in the native query as well. #15020

LakshSingla

LGTM once @cryptoe's comments have been addressed. The major concern behind the correct optimization of the filter is present in the native stack as well (occurs while query planning), and is unrelated to enabling unnest in MSQ.

...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java

somu-imply · 2023-09-21T22:31:15Z

Addressed the comments and added the test case

…t/Filter DS

cryptoe

Changes LGTM!!

somu-imply added 3 commits August 15, 2023 08:38

Adding unnest to MSQ

416a139

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

5037157

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

5916cfd

soumyava added Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 WIP labels Aug 23, 2023

somu-imply added 4 commits August 23, 2023 20:37

Adding filtered data source + nested querying on unnest works in MSQ

eb024d0

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

d3c1e76

Removing Unnest feature

b4a8c62

Cleaning up code

96bbaa4

somu-imply changed the title ~~Unnest msq~~ Unnest now works on MSQ Aug 24, 2023

somu-imply added 3 commits August 24, 2023 16:40

removing a test added by mistake

a3599b7

Handling failed CI for useDefault=false

fab0425

tmp

b17186e

somu-imply commented Aug 25, 2023

View reviewed changes

sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java Outdated Show resolved Hide resolved

somu-imply added 4 commits August 25, 2023 16:30

Do not need the shift

31f283b

Updating to accept JOIN under unnest

be06d35

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

12bd9fa

FilteredDataSource and unnestdata source can have joins underneath an…

5096be3

…d can have non empty side channels

somu-imply marked this pull request as ready for review August 29, 2023 15:27

somu-imply added 2 commits August 29, 2023 08:58

Removing stale comment

e249736

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

79c1cd4

somu-imply commented Aug 29, 2023

View reviewed changes

sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java Outdated Show resolved Hide resolved

soumyava added Area - Querying and removed WIP labels Aug 29, 2023

cryptoe reviewed Sep 4, 2023

View reviewed changes

...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java Outdated Show resolved Hide resolved

cryptoe reviewed Sep 4, 2023

View reviewed changes

somu-imply added 2 commits September 5, 2023 11:32

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

1dd7933

Adding insert and replace tests and fixing broadcast inputs for filte…

53db2ff

…red data source

LakshSingla reviewed Sep 5, 2023

View reviewed changes

Refactor 1

948668c

somu-imply added 5 commits September 6, 2023 18:24

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

8d33ed9

Adding the unnest engine feature back

a4659d3

Making the feature always true

7a30bfc

Moving the engine feature from planner context to individual engines.…

d942772

… That way Unnest can be turned on/off for individual engine such as MSQ if needed

Revert "Moving the engine feature from planner context to individual …

d0a423b

…engines. That way Unnest can be turned on/off for individual engine such as MSQ if needed" This reverts commit d942772.

LakshSingla mentioned this pull request Sep 8, 2023

Field writers for numerical arrays #14900

Merged

10 tasks

LakshSingla reviewed Sep 11, 2023

View reviewed changes

somu-imply added 2 commits September 15, 2023 08:23

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

7119ed0

Removing an unnecessary check in dataSourcePlan + pushing feature ins…

4588e30

…ide engines

github-actions bot added the Area - Batch Ingestion label Sep 15, 2023

LakshSingla reviewed Sep 15, 2023

View reviewed changes

somu-imply added 3 commits September 15, 2023 11:27

Updating engine in tests

3b2bdfb

Adding new datasource to msq tests

32dbeba

Merge remote-tracking branch 'upstream/master' into Unnest_MSQ

ef2424f

cryptoe reviewed Sep 21, 2023

View reviewed changes

LakshSingla approved these changes Sep 21, 2023

View reviewed changes

LakshSingla reviewed Sep 21, 2023

View reviewed changes

...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java Outdated Show resolved Hide resolved

...sions-core/multi-stage-query/src/main/java/org/apache/druid/msq/querykit/DataSourcePlan.java Outdated Show resolved Hide resolved

Updating code and test cases

77905d4

Removed the subqueryDefBuilder to use the one from basePlan for Unnes…

d4a6c7c

…t/Filter DS

cryptoe approved these changes Sep 25, 2023

View reviewed changes

cryptoe merged commit c184b52 into apache:master Sep 25, 2023
74 checks passed

LakshSingla added this to the 28.0 milestone Oct 12, 2023

LakshSingla mentioned this pull request Nov 4, 2023

[DRAFT] 28.0.0 release notes #15326

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unnest now works on MSQ #14886

Unnest now works on MSQ #14886

somu-imply commented Aug 21, 2023 •

edited

Loading

cryptoe left a comment

cryptoe Sep 4, 2023

LakshSingla Sep 5, 2023

LakshSingla Sep 5, 2023

somu-imply Sep 5, 2023

LakshSingla Sep 5, 2023 •

edited

Loading

somu-imply Sep 5, 2023

LakshSingla Sep 6, 2023

LakshSingla Sep 5, 2023

LakshSingla Sep 5, 2023

LakshSingla Sep 5, 2023

LakshSingla Sep 11, 2023

LakshSingla Sep 15, 2023

somu-imply Sep 15, 2023 •

edited

Loading

LakshSingla Sep 20, 2023

LakshSingla Sep 11, 2023

somu-imply commented Sep 15, 2023

LakshSingla left a comment

LakshSingla Sep 15, 2023

cryptoe left a comment

cryptoe Sep 21, 2023

LakshSingla Sep 21, 2023 •

edited

Loading

LakshSingla Sep 21, 2023 •

edited

Loading

LakshSingla commented Sep 21, 2023

LakshSingla commented Sep 21, 2023

LakshSingla left a comment

somu-imply commented Sep 21, 2023

cryptoe left a comment

		@@ -123,6 +124,7 @@ public static List<RelOptRule> rules(PlannerContext plannerContext)
		retVal.add(DruidFilterUnnestRule.DruidProjectOnUnnestRule.instance());
		}

	Math.max(minStageNumber, subQueryDefBuilder.getNextStageNumber()),
	minStageNumber,

Unnest now works on MSQ #14886

Unnest now works on MSQ #14886

Conversation

somu-imply commented Aug 21, 2023 • edited Loading

cryptoe 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

LakshSingla Sep 5, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somu-imply Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somu-imply commented Sep 15, 2023

LakshSingla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cryptoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LakshSingla Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

LakshSingla Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

LakshSingla commented Sep 21, 2023

LakshSingla commented Sep 21, 2023

LakshSingla left a comment

Choose a reason for hiding this comment

somu-imply commented Sep 21, 2023

cryptoe left a comment

Choose a reason for hiding this comment

somu-imply commented Aug 21, 2023 •

edited

Loading

LakshSingla Sep 5, 2023 •

edited

Loading

somu-imply Sep 15, 2023 •

edited

Loading

LakshSingla Sep 21, 2023 •

edited

Loading

LakshSingla Sep 21, 2023 •

edited

Loading