Window planning: use collation traits, improve subquery logic. by gianm · Pull Request #13902 · apache/druid

gianm · 2023-03-08T18:31:31Z

SQL changes:

Attach RelCollation (sorting) trait to any PartialDruidQuery
that ends in AGGREGATE or AGGREGATE_PROJECT. This allows planning to
take advantage of the fact that Druid sorts by dimensions when
doing aggregations.
Windowing: inspect RelCollation trait from input, and insert naiveSort
if, and only if, necessary.
Windowing: add support for Project after Window, when the Project
is a simple mapping. Helps eliminate subqueries.
DruidRules: update logic for considering subqueries to reflect that
subqueries are not required to be GroupBys, and that we have a bunch
of new Stages now. With all of this evolution that has happened, the
old logic didn't quite make sense.

Native changes:

Use merge sort (stable) rather than quicksort when sorting
RowsAndColumns. Makes it easier to write test cases for plans that
involve re-sorting the data.

SQL changes: 1) Attach RelCollation (sorting) trait to any PartialDruidQuery that ends in AGGREGATE or AGGREGATE_PROJECT. This allows planning to take advantage of the fact that Druid sorts by dimensions when doing aggregations. 2) Windowing: inspect RelCollation trait from input, and insert naiveSort if, and only if, necessary. 3) Windowing: add support for Project after Window, when the Project is a simple mapping. Helps eliminate subqueries. 4) DruidRules: update logic for considering subqueries to reflect that subqueries are not required to be GroupBys, and that we have a bunch of new Stages now. With all of this evolution that has happened, the old logic didn't quite make sense. Native changes: 1) Use merge sort (stable) rather than quicksort when sorting RowsAndColumns. Makes it easier to write test cases for plans that involve re-sorting the data.

imply-cheddar · 2023-03-08T22:39:54Z

        plannerContext.setPlanningError(
-            "SQL query is a scan and requires order by on a datasource[%s], which is not supported.",
+            "SQL query requires order by on non-concrete datasource, which is not supported.",
            dataSource


you dropped the interpolation of the datasource value. In a complex query that does lots and lots of stuff, not having anything interpolated that tells you what you did wrong makes it completely impossible to actually fix your query.

Oops. That was a mistake, you can tell because dataSource is still in the call. Added back the %s.

imply-cheddar · 2023-03-08T22:45:05Z

+      sortColumns.addAll(group.getOrdering());

+      // Add sorting and partitioning if needed.
+      if (!sortMatches(ImmutableList.copyOf(priorSortColumns), ImmutableList.copyOf(sortColumns))) {


Why create the copy into a list? Given that sortMatches is doing a prefix match anyway, you might as well just pass in 2 iterators?

I changed it to use iterators.

imply-cheddar · 2023-03-08T22:46:28Z

+  private static boolean sortMatches(
+      final List<ColumnWithDirection> priorSort,
+      final List<ColumnWithDirection> currentSort
+  )
+  {
+    return currentSort.size() <= priorSort.size() && currentSort.equals(priorSort.subList(0, currentSort.size()));
+  }


This is an pretty high garbage way of doing things, and it doesn't seem much simpler than just having 2 iterators and walking them together.

Sure it's simpler, it's only one line vs, like, 5 😛

Anyway, I changed it.

imply-cheddar · 2023-03-08T22:49:04Z

      // The ordering required for partitioning is actually not important for the semantics.  However, it *is*
      // important that it be consistent across the query.  Because if the incoming data is sorted descending
      // and we try to partition on an ascending sort, we will think the data is not sorted correctly


I have a feeling like this comment has gone stale with your changes?

OK, it doesn't seem to be saying something useful, so I deleted it.

imply-cheddar · 2023-03-08T22:53:58Z

+      switch (fieldCollation.getDirection()) {
+        case ASCENDING:
+        case STRICTLY_ASCENDING:
+          direction = ColumnWithDirection.Direction.ASC;
+          break;
+
+        case DESCENDING:
+        case STRICTLY_DESCENDING:
+          direction = ColumnWithDirection.Direction.DESC;
+          break;
+      }


If one of the fieldCollation values is using CLUSTERED direction, I think that this code will assume that the data is not sorted from that point forward and return? Is that intended behavior? If so, it would be a lot more explicit to have a case in the switch that covers it and returns retVal immediately instead of relying on a null value and then falling through.

Alternatively, we could generate an error message? I'm unsure why this would receive a CLUSTERED direction, so not sure which is correct.

Adjusted it to return retVal immediately. I'm not sure if we'll ever see CLUSTERED order, but if we do, we should treat it as unsorted.

imply-cheddar · 2023-03-08T23:00:10Z

+# Not correct: there should actually be results here. Therefore, currently, this test only verifies that the
+# query is planned as expected, not that the results are correct.
+expectedResults: []


Is the test not failing? I would expect this test to be failing saying that the results are different. If we want plan-only tests, I think I'd want the type to become operatorPlanValidation and then the test harness to only look at the expected operators and ignore the results.

It is not failing. I am not sure why the results are empty. The native query looks good to me so I figured something was wrong with the execution part. I looked into it a little but in that time wasn't able to figure out where the results were getting dropped. One difference here is that the base query type is scan, all other tests have a base query type groupBy. Is that something that is supposed to work?

Oooooh, yeah, scan doesn't work because it doesn't actually produce a good RowSignature for the ArrayListSegment (this is addressed some in #13773). So that's likely the culprit. Though, I would've expected that to generate an exception...

imply-cheddar · 2023-03-09T03:00:16Z

+    final LinkedHashSet<ColumnWithDirection> retVal = new LinkedHashSet<>();
+
+    for (RelFieldCollation fieldCollation : collation.getFieldCollations()) {
+      ColumnWithDirection.Direction direction;


Nit: I think you can make this final.

abhishekagarwal87 · 2023-04-14T08:37:08Z

+        // Scan cannot ORDER BY non-concrete datasources on _any_ column.
        plannerContext.setPlanningError(
-            "SQL query is a scan and requires order by on a datasource[%s], which is not supported.",
+            "SQL query requires order by on non-concrete datasource [%s], which is not supported.",


I was looking at this PR for some other reason. I don't think we should be using "concrete" since that concept is not well understood by the users. As a user, it will not be clear to me to know what a non-concrete data source means. what do you think?

The message was removed anyway in #13965: https://github.com/apache/druid/pull/13965/files#r1145293604.

Although, TBH, I don't understand why it was removed. The comment "Since we are using a table data source and not a query data source now the isConcrete() check is not needed" doesn't really make sense to me. It's still possible for the dataSource to be non-concrete at this point in the code. I tried a test query that generates a non-concrete datasource at this point in the code, and the error you get is like this:

Time-ordering on scan queries is only supported for queries with segment specs of type MultipleSpecificSegmentSpec

It's from native execution, not from SQL planning, since the query does now pass the SQL planner. IMO in terms of clarity, it's even worse 😛

Test query is:

select __time as t, m1 from (select __time, m1 from druid.foo where __time >= timestamp '1970-01-01 00:00:00') where (m1 in (select distinct m1 from druid.foo)) order by 1 limit 1

I think we could address this by restoring a check here, but instead of checking isConcrete, check isConcreteBased. That'd allow joins on concrete stuff, but not subqueries. Then for the message, we could do something like:

ORDER BY is only supported for __time, and only on regular tables (not subqueries)

Or, we could spend the time supporting order-by for all scans ✨

Further adjusts logic in DruidRules that was previously adjusted in apache#13902. The reason for the original change was that the comment "Subquery must be a groupBy, so stage must be >= AGGREGATE" was no longer accurate. Subqueries do not need to be groupBy anymore; they can really be any type of query. If I recall correctly, the change was needed for certain window queries to be able to plan on top of Scan queries. However, this impacts performance negatively, because it causes many additional outer-query scenarios to be considered, which is expensive. So, this patch updates the matching logic to consider fewer scenarios. The skipped scenarios are ones where we expect that, for one reason or another, it isn't necessary to consider a subquery.

* SQL planning: Consider subqueries in fewer scenarios. Further adjusts logic in DruidRules that was previously adjusted in #13902. The reason for the original change was that the comment "Subquery must be a groupBy, so stage must be >= AGGREGATE" was no longer accurate. Subqueries do not need to be groupBy anymore; they can really be any type of query. If I recall correctly, the change was needed for certain window queries to be able to plan on top of Scan queries. However, this impacts performance negatively, because it causes many additional outer-query scenarios to be considered, which is expensive. So, this patch updates the matching logic to consider fewer scenarios. The skipped scenarios are ones where we expect that, for one reason or another, it isn't necessary to consider a subquery. * Remove unnecessary escaping. * Fix test.

* SQL planning: Consider subqueries in fewer scenarios. Further adjusts logic in DruidRules that was previously adjusted in apache#13902. The reason for the original change was that the comment "Subquery must be a groupBy, so stage must be >= AGGREGATE" was no longer accurate. Subqueries do not need to be groupBy anymore; they can really be any type of query. If I recall correctly, the change was needed for certain window queries to be able to plan on top of Scan queries. However, this impacts performance negatively, because it causes many additional outer-query scenarios to be considered, which is expensive. So, this patch updates the matching logic to consider fewer scenarios. The skipped scenarios are ones where we expect that, for one reason or another, it isn't necessary to consider a subquery. * Remove unnecessary escaping. * Fix test.

* SQL planning: Consider subqueries in fewer scenarios. Further adjusts logic in DruidRules that was previously adjusted in #13902. The reason for the original change was that the comment "Subquery must be a groupBy, so stage must be >= AGGREGATE" was no longer accurate. Subqueries do not need to be groupBy anymore; they can really be any type of query. If I recall correctly, the change was needed for certain window queries to be able to plan on top of Scan queries. However, this impacts performance negatively, because it causes many additional outer-query scenarios to be considered, which is expensive. So, this patch updates the matching logic to consider fewer scenarios. The skipped scenarios are ones where we expect that, for one reason or another, it isn't necessary to consider a subquery. * Remove unnecessary escaping. * Fix test.

gianm added the Area - SQL label Mar 8, 2023

github-advanced-security AI found potential problems Mar 8, 2023

View reviewed changes

Comment thread sql/src/main/java/org/apache/druid/sql/calcite/rel/Windowing.java Fixed

imply-cheddar reviewed Mar 8, 2023

View reviewed changes

gianm added 5 commits March 8, 2023 15:30

Merge branch 'master' into window-updates

d7db8fd

Changes from review.

6ebeba3

Mark the bad test as failing.

7f99d5a

Additional update.

dbb5636

Fix failingTest.

6817d86

imply-cheddar approved these changes Mar 9, 2023

View reviewed changes

gianm added 3 commits March 8, 2023 22:05

Merge branch 'master' into window-updates

afdbf2a

Fix tests.

5535c90

Mark a var final.

37c1fa3

gianm merged commit bf39b4d into apache:master Mar 9, 2023

gianm deleted the window-updates branch March 9, 2023 23:48

clintropolis mentioned this pull request Mar 16, 2023

fix join and unnest planning to ensure that duplicate join prefixes are not used #13943

Merged

4 tasks

clintropolis added this to the 26.0 milestone Apr 10, 2023

techdocsmith mentioned this pull request Apr 12, 2023

[DRAFT] 26.0.0 release notes #14064

Closed

abhishekagarwal87 reviewed Apr 14, 2023

View reviewed changes

gianm mentioned this pull request Apr 20, 2023

SQL planning: Consider subqueries in fewer scenarios. #14123

Merged

gianm mentioned this pull request Apr 25, 2023

Community roadmap 2023 #14157

Open

Conversation

gianm commented Mar 8, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gianm Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gianm Apr 20, 2023 •

edited

Loading