Upgrade Calcite to 1.21 by jon-wei · Pull Request #8566 · apache/druid

jon-wei · 2019-09-21T03:07:37Z

This PR updates Calcite to 1.21.

Upgrading also fixes #8266

This patch introduces incompatible behavior in two areas:

The Calcite optimizer can remove the ORDER BY from subqueries when there is no limit or offset (https://issues.apache.org/jira/browse/CALCITE-2798). An example in our unit tests is shown here: https://github.com/apache/incubator-druid/pull/8566/files#diff-190f8eaf9dd09e8d7ae0564804286afcR808
The Calcite optimizer can make certain optimizations that assume SQL-compatible null handling behavior, e.g.:
https://github.com/apache/incubator-druid/pull/8566/files#diff-190f8eaf9dd09e8d7ae0564804286afcR1830
https://github.com/apache/incubator-druid/pull/8566/files#diff-190f8eaf9dd09e8d7ae0564804286afcR1888

The web console relies on the subquery ORDER BY being respected (it can wrap queries made in the query view with an outer query that has a limit), so this patch adds an internal query context flag that wraps a query with a limit-only LogicalSort if specified.

Before including this version upgrade in a release, I think we should expose the SQL compatible null handling mode in our docs (#4349) and update the web console to use the new context flag for applying limits.

This PR has:

been self-reviewed.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths.

clintropolis

overall changes lgtm, I think we def need to call out in release notes the changes in SQL for Druid default non-standard null handling behavior (as well as consider switching to SQL compatible being the default), but still seems worth it.

clintropolis · 2019-10-08T23:54:22Z

clintropolis · 2019-10-09T00:01:57Z

Hmm, this seems strange and sort of lame in terms of the produced expression, but i guess more correct in terms of the types involved for the SQL?

Something should be collapsing these casts. Could you add a comment to the test that we're aware the behavior here is weird? Maybe a future developer will see that and it will inspire them.

clintropolis · 2019-10-09T00:04:30Z

can drop ternary condition

clintropolis · 2019-10-09T00:15:12Z

why decimal instead of like bigint or... something?

clintropolis · 2019-10-09T00:15:57Z

could possiblyWrapRootWithOuterLimitFromContext just return root.rel instead of null to collapse these near identical planner transform blocks?

clintropolis · 2019-10-09T00:19:04Z

This test was testing the bloom_filter_test expression macro function rather than a generic bloom filter sql test, so I think the query needs to change to continue to do that, or this test be removed.

clintropolis · 2019-10-09T00:20:14Z

why no final?

clintropolis · 2019-10-09T00:21:51Z

should this be documented somewhere or just leave it hidden since I guess is mostly to be friendly to web console?

I think leaving it undocumented makes sense, since it's meant to be internal. End users should add a LIMIT to their queries. If we discover use cases where it makes sense to expose it then we could do it then.

I think it would be better to document internal parameters somewhere rather than depending on human memory. But I think we can do in a follow-up pr.

I think this should be in the docs, it is a super useful parameter IMO even outside the web console

clintropolis

👍

jihoonson · 2019-11-20T20:07:01Z

@jon-wei thanks. I'm reviewing this PR.

gianm · 2019-09-24T17:09:28Z

                               .dataSource(CalciteTests.DATASOURCE1)
                               .intervals(querySegmentSpec(Filtration.eternity()))
-                               .filters(selector("dim2", "0", null))
+                               .filters(bound("dim2", "0", "0", false, false, null, StringComparators.NUMERIC))


This means that instead of casting 0 to "0" the planner is now casting dim2 to numeric. Performance will be worse but it is technically more correct. (What if dim2 was "0.0"?) So, it sounds good to me.

gianm · 2019-09-24T17:14:15Z

+                .columns(ImmutableList.of("dim1"))
                .limit(2)
-                .order(ScanQuery.Order.DESCENDING)
+                .order(ScanQuery.Order.NONE)


Technically correct (the standard allows it) but now this test misses the point of what it was originally trying to validate (that the wrapping technique used by the web console works).

I suggest keeping this test, but adding a comment like the one you have in testSelectProjectionFromSelectSingleColumnDescending. And then adding another test that uses the inner query here, plus the new sqlOuterLimit functionality, to verify that the Druid query generated in that case is a good one.

Added a comment, and an additional query within this test that uses the outer context limit instead. One difference there is that the new query includes __time within the results (otherwise the scan query rejects the query, since it is ordering on __time)

gianm · 2019-09-24T17:17:41Z

  {
    // Regression test for https://github.com/apache/incubator-druid/issues/7768.

+    // After upgrading to Calcite 1.21, the results return in the wrong order, the ORDER BY __time DESC


Similar comment as above: the behavior is technically correct, and I think we should roll with it. For this test, I'd suggest:

keep the query the same

remove this comment

add a comment that says we're verifying that the inner order by is stripped, as the standard allows

I revised the comment here

gianm · 2019-09-24T17:18:12Z

+                          )
+                      )
+                  )
+                  //.filters(expressionFilter("case_searched((\"dim2\" == 'a'),1,isnull(\"dim2\"))"))


Please don't include commented-out code.

This has been removed

gianm · 2019-10-01T16:26:51Z

+        ImmutableList.of(new Object[]{0L})
+    );
+    /*
+    The SQL query in this test planned to the following Druid query in Calcite 1.17.


What does the test case look like if NULLIF(dim2, 'a') = NULL is replaced with NULLIF(dim2, 'a') IS NULL? The new behavior seems like a valid optimization, since <anything> = NULL is false. We could update the SQL docs appropriately to point out that you should avoid = NULL and use IS NULL no matter what your null handling mode is.

Btw, please don't include commented-out code.

There is a version that uses IS NULL instead in testNullEmptyStringEquality

gianm · 2019-11-20T19:51:01Z

I think leaving it undocumented makes sense, since it's meant to be internal. End users should add a LIMIT to their queries. If we discover use cases where it makes sense to expose it then we could do it then.

gianm · 2019-11-20T20:02:01Z

+                              not(selector("dim2", "a", null))
+                          )
+                      )
+                  )


The new filter is dim2 = 'a' OR (dim2 IS NULL AND dim2 != 'a'). It seems a bit… weird. Ideally it should be simplified to dim2 = 'a' OR dim2 IS NULL. It's fine for now, but could you add a comment about this?

Added a comment on the unnecessary dim2 != 'a'

gianm · 2019-11-20T20:17:22Z

Something should be collapsing these casts. Could you add a comment to the test that we're aware the behavior here is weird? Maybe a future developer will see that and it will inspire them.

gianm · 2019-11-20T20:25:40Z

  }

+  /**
+   * In Calcite 1.17, this test worked, but after upgrading to Calcite 1.21, this query fails with:


Please do two things in response to this:

Keep this test as-is, but add a new, second test that is the same basic query shape but doesn't trip the dim1 is ambiguous thing. Maybe rename one of them, or group by ordinals. Testing project after sort is still important, so there's value in this.

If you think this is a Calcite bug, raise a Calcite issue at: https://issues.apache.org/jira/projects/CALCITE/

I added a new test:

@Test public void testProjectAfterSort3WithoutAmbiguity() throws Exception { // This query is equivalent to the one in testProjectAfterSort3 but renames the second grouping column // to avoid the ambiguous name exception. The inner sort is also optimized out in Calcite 1.21. testQuery( "select copydim1 from (select dim1, dim1 AS copydim1, count(*) cnt from druid.foo group by dim1, dim1 order by cnt)", ImmutableList.of( GroupByQuery.builder() .setDataSource(CalciteTests.DATASOURCE1) .setInterval(querySegmentSpec(Filtration.eternity())) .setGranularity(Granularities.ALL) .setDimensions( dimensions( new DefaultDimensionSpec("dim1", "d0") ) ) .setContext(QUERY_CONTEXT_DEFAULT) .build() ), ImmutableList.of( new Object[]{""}, new Object[]{"1"}, new Object[]{"10.1"}, new Object[]{"2"}, new Object[]{"abc"}, new Object[]{"def"} ) ); }

As with the other project-after-sort tests, the inner ordering is optimized out.

I'll need to do some more investigation to see if this is a bug or not.

gianm · 2019-11-20T20:26:03Z

                               .aggregators(aggregators(
                                   new CountAggregatorFactory("a0")
                               ))
+                               // after upgrading to Calcite 1.21, expressions like sin(pi/6) that only reference


jihoonson · 2019-11-20T20:03:40Z


 public class DruidOperatorTable implements SqlOperatorTable
 {
+  private static final EmittingLogger log = new EmittingLogger(DruidOperatorTable.class);


Unused variable?

Removed unused logger

jihoonson · 2019-11-20T20:04:54Z

+import org.apache.druid.segment.DimensionHandlerUtils;
 import org.apache.druid.sql.calcite.rel.DruidConvention;
 import org.apache.druid.sql.calcite.rel.DruidRel;
+import org.checkerframework.checker.nullness.qual.Nullable;


Hmm I think we usually use javax.annotation.Nullable.

Ah, changed to the right Nullable

jihoonson · 2019-11-20T20:17:03Z

I think it would be better to document internal parameters somewhere rather than depending on human memory. But I think we can do in a follow-up pr.

jihoonson · 2019-11-20T20:28:08Z

+        "SELECT COUNT(*)\n"
+        + "FROM druid.foo\n"
+        + "WHERE NULLIF(dim2, 'a') = null",
+        ImmutableList.of(),


Missing query plan verification?

No native Druid query is actually generated for this, the filter would never be true and Calcite just returns a count of 0

jihoonson · 2019-11-20T20:28:44Z

+        + "WHERE NULLIF(dim2, 'a') = null",
+        ImmutableList.of(),
+        NullHandling.replaceWithDefault() ?
+        // Matches everything but "abc"


Looks like the comment is wrong?

Fixed this area, the ternary was also unnecessary

jihoonson

LGTM

gianm

👍 after latest changes, thanks @jon-wei

* Upgrade Calcite to 1.21 * Checkstyle, test fix' * Exclude calcite yaml deps, update license.yaml * Add method for exception chain handling * Checkstyle * PR comments, Add outer limit context flag * Revert project settings change * Update subquery test comment * Checkstyle fix * Fix test in sql compat mode * Fix test * Fix dependency analysis * Address PR comments * Checkstyle * Adjust testSelectStarFromSelectSingleColumnWithLimitDescending

Upgrade Calcite to 1.21

7e56e00

jon-wei added WIP Area - SQL Area - Dependencies labels Sep 21, 2019

jon-wei added 4 commits September 23, 2019 15:44

Checkstyle, test fix'

608812c

Exclude calcite yaml deps, update license.yaml

8d8d741

Add method for exception chain handling

bddac22

Checkstyle

62c6132

clintropolis reviewed Oct 9, 2019

View reviewed changes

jon-wei added the Release Notes label Oct 17, 2019

jon-wei force-pushed the left_wip branch from a7ebc8b to 73f987d Compare October 17, 2019 20:25

PR comments, Add outer limit context flag

e8dd078

jon-wei force-pushed the left_wip branch from 73f987d to e8dd078 Compare October 17, 2019 20:45

jon-wei added 2 commits October 17, 2019 13:46

Revert project settings change

6264b71

Update subquery test comment

4f5cf77

jon-wei added the Incompatible label Oct 17, 2019

Checkstyle fix

0e80ce1

clintropolis approved these changes Oct 18, 2019

View reviewed changes

jon-wei added 4 commits October 17, 2019 18:37

Fix test in sql compat mode

4567a56

Fix test

60f1b76

Fix dependency analysis

dc72b0f

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

ea6ff84

jon-wei added the Area - Web Console label Oct 18, 2019

clintropolis mentioned this pull request Nov 5, 2019

optimize numeric column null value checking for low filter selectivity (more rows) #8822

Merged

2 tasks

jon-wei added 2 commits November 14, 2019 14:53

erge remote-tracking branch 'upstream/master' into left_wip

d21eb04

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

ad26af1

jon-wei removed the WIP label Nov 20, 2019

gianm reviewed Nov 20, 2019

View reviewed changes

jihoonson reviewed Nov 20, 2019

View reviewed changes

jon-wei added 3 commits November 20, 2019 15:31

Address PR comments

59486e6

Checkstyle

adf49e4

Adjust testSelectStarFromSelectSingleColumnWithLimitDescending

66d23a4

jihoonson approved these changes Nov 21, 2019

View reviewed changes

gianm approved these changes Nov 21, 2019

View reviewed changes

gianm merged commit dc6178d into apache:master Nov 21, 2019

clintropolis mentioned this pull request Nov 21, 2019

fix bug with sqlOuterLimit, use sqlOuterLimit in web console #8919

Merged

7 tasks

jon-wei added this to the 0.17.0 milestone Dec 17, 2019

clintropolis mentioned this pull request Dec 23, 2019

sql support for dynamic parameters #6974

Merged

jon-wei mentioned this pull request Dec 28, 2019

0.17.0 release notes #9066

Closed

clintropolis added the Area - Null Handling label Jan 9, 2020

clintropolis mentioned this pull request Feb 26, 2020

Regression: SQL query with multiple similar CASE clauses cannot be translated to correct native json format #9412

Closed

gianm mentioned this pull request Sep 1, 2020

Critical Calcite StackOverflowError in 1.21.0 causing failure in WHERE with too many conditions #10225

Open

Conversation

jon-wei commented Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clintropolis left a comment

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!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Nov 20, 2019

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!

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!

jon-wei commented Sep 21, 2019 •

edited

Loading