Clone Calcite planner to access validator by paul-rogers · Pull Request #12708 · apache/druid

paul-rogers · 2022-06-25T23:16:17Z

This is a breakout from PR #12636: Single Pass through the Calcite planner. This PR adds just one item: a clone of the Calcite planner, lightly converted to Druid's coding style, with a method to provide access to the validator. The only tricky bit is the validator itself. While #12636 cloned the validator, this PR uses a sneaky approach: it defines DruidSqlValidator as a subclass of the unreachable CalciteSqlValidator. To do that, it lives in the same Calcite name space as CalciteSqlValidator. Some checkstyle hacking was needed to allow this odd state of affairs.

There is zero change in functionality: everything works exactly as before. The goal here is simply to review the replacement of the Druid home-grown validator with the Calcite-provided one. This will then allow, in a later PR, to combine the validation and plan steps in the Druid planner.

Testing is handled by the large number of existing SQL tests, including integration tests.

This PR has:

been self-reviewed.
added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
been tested in a test Druid cluster.

paul-rogers · 2022-06-30T16:27:42Z

Build is clean, except for a known flaky IT: 37272.52 (Compile=openjdk8, Run=openjdk8) perfect rollup parallel batch index integration test with Indexer, reported as Issue #12692. Ready for review.

Done in preparation for the "single-pass" planner.

paul-rogers · 2022-07-02T05:59:34Z

Ready for review. Different flaky IT in the latest build: (Compile=openjdk8, Run=openjdk8) query retry integration test for missing segments, described in Issue #12733.

clintropolis · 2022-07-08T20:55:31Z

+   * In some future this could perhaps re-use some work done by {@link #validate(boolean)}
   * instead of repeating it, but that day is not today.
   */
+  // RelConversionException is no longer thrown, but removing it causes


nit: is it really that many files?

Yes! It expands ever-outward. Is good to clean up, but as a separate PR to keep this one small.

got it, was curious because on the surface it only looks like SqlLifecycle and some tests. As long as we don't forget about it i think its fine.

Right. From SqlLifecycle, the exception is exposed to the callers, which include the tests. Many tests then expose the exception in their test function signatures. So, to fully clean up, we have to clean up many test functions. That's the work to be done in another PR.

abhishekagarwal87 · 2022-07-09T09:13:09Z

This PR also paves the way to support multiple SQL statements now that we have forked the PlannerImpl class. We could change the CalcitePlanner to call parseStmtList() instead of parseStmt(). The calcite parser supports multiple SQL statements but the planner doesn't expose it.

rohangarg

Thanks for the changes! 👍
left some comments

rohangarg · 2022-07-09T12:27:29Z

    System.setProperty("saffron.default.collation.name", StringUtils.format("%s$en_US", charset));
+
+    // The following are the current names. See org.apache.calcite.config.CalciteSystemProperty
+    // https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/config/CalciteSystemProperty.java


thanks for checking up on this 👍 - can we also drop setting the old properties?

Probably. I was being super-cautious to limit changes in this PR.

There is some mystery bug I never did solve: somewhere along the line Calcite stopped using our property values. If found this issue when trying to figure out why Calcite was ignoring our values.

rohangarg · 2022-07-09T12:51:33Z

+  }
+
+  @Override
+  public RelNode transform(


is the dropping of RelConversionException from the Planner methods intended? I tried but could not figure out the reason for it

As it turns out, Calcite declares an exception which is not thrown. Our IntelliJ inspections then refuse to allow this and require that we rectify Calcite's error. That causes the cascading issues mentioned elsewhere. This is a start to say that the exception isn't really thrown.

paul-rogers · 2022-07-13T22:46:44Z

@abhishekagarwal87, good point:

This PR also paves the way to support multiple SQL statements now that we have forked the PlannerImpl class. We could change the CalcitePlanner to call parseStmtList() instead of parseStmt(). The calcite parser supports multiple SQL statements but the planner doesn't expose it.

The trick is that planning more than one statement at a time creates a race condition. In a normal DB, the following would fail:

CREATE TABLE foo ...;
SELECT * FROM foo

If we plan both statements up front, the second will fail because table foo doesn't exist yet. Once we support INSERT, we have the same issue, since INSERT can create a table. Still, if we had ALTER SESSION SET <option> = <value> (in place of context variables), it would be handy to do:

ALTER SESSION SET "priority" = 1;
SELECT ...

One solution that some tools use is to first split the statements at the semi-colon delimiter, then plan and execute them one by one. That eliminates the race conditions. For the above, we could create a mini per-request "session" to track the options, seeded with query context values.

paul-rogers · 2022-07-13T22:49:52Z

Latest build failed with what I assume to be a flaky IT: (Compile=openjdk8, Run=openjdk8) leadership and high availability integration tests. The same IT passed in earlier builds, and nothing in the latest commit should affect the servers. The error says one of the servers failed to become ready.

Perhaps a committer can kindly restart that one test to see if we get lucky this time.

clintropolis

👍

clintropolis · 2022-07-14T02:35:47Z

+   * In some future this could perhaps re-use some work done by {@link #validate(boolean)}
   * instead of repeating it, but that day is not today.
   */
+  // RelConversionException is no longer thrown, but removing it causes


got it, was curious because on the surface it only looks like SqlLifecycle and some tests. As long as we don't forget about it i think its fine.

abhishekagarwal87 · 2022-07-14T07:47:13Z

@abhishekagarwal87, good point:

This PR also paves the way to support multiple SQL statements now that we have forked the PlannerImpl class. We could change the CalcitePlanner to call parseStmtList() instead of parseStmt(). The calcite parser supports multiple SQL statements but the planner doesn't expose it.

The trick is that planning more than one statement at a time creates a race condition. In a normal DB, the following would fail:
CREATE TABLE foo ...;
SELECT * FROM foo
If we plan both statements up front, the second will fail because table foo doesn't exist yet. Once we support INSERT, we have the same issue, since INSERT can create a table. Still, if we had ALTER SESSION SET <option> = <value> (in place of context variables), it would be handy to do:
ALTER SESSION SET "priority" = 1;
SELECT ...
One solution that some tools use is to first split the statements at the semi-colon delimiter, then plan and execute them one by one. That eliminates the race conditions. For the above, we could create a mini per-request "session" to track the options, seeded with query context values.

Indeed. statements like ALTER SESSION was infact the very reason why we were exploring supporting multiple statements. So users can write the SQL query like

SET CONTEXT timeout=30000;
SET CONTEXT priority=1;
SELECT COUNT(*) from the_awesome_table

Takes us one step closer to just submitting SQL as a string to Druid. Right now, that SQL needs to be wrapped up inside a JSON.

paul-rogers force-pushed the 220625-validator branch from 78abe7f to 8ae28b4 Compare June 28, 2022 18:57

Clone the Calcite planner to provide access to validator

5669dbf

Done in preparation for the "single-pass" planner.

paul-rogers force-pushed the 220625-validator branch from 5c88bea to 5669dbf Compare June 30, 2022 16:31

clintropolis reviewed Jul 8, 2022

View reviewed changes

clintropolis added Area - SQL Refactoring labels Jul 8, 2022

rohangarg reviewed Jul 9, 2022

View reviewed changes

Revisions from review comments

8b7935b

clintropolis approved these changes Jul 14, 2022

View reviewed changes

clintropolis merged commit ee15c23 into apache:master Jul 15, 2022

paul-rogers mentioned this pull request Jul 15, 2022

Today is that day - Single pass through Calcite planner #12636

Merged

5 tasks

abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022

Conversation

paul-rogers commented Jun 25, 2022

Uh oh!

paul-rogers commented Jun 30, 2022

Uh oh!

paul-rogers commented Jul 2, 2022

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!

Uh oh!

abhishekagarwal87 commented Jul 9, 2022

Uh oh!

rohangarg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

paul-rogers commented Jul 13, 2022

Uh oh!

paul-rogers commented Jul 13, 2022

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhishekagarwal87 commented Jul 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants