[BEAM-7832] Add ZetaSQL as a dialect in BeamSQL #9210

amaliujia · 2019-07-31T23:02:09Z

Support ZetaSQL(https://github.com/google/zetasql) as a dialect in BeamSQL.

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

Post-Commit Tests Status (on master branch)

Lang	Apex	Dataflow	Gearpump	Samza
Go	---	---	---	---
Java
Python	---		---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

amaliujia · 2019-07-31T23:35:01Z

Run Java PreCommit

amaliujia · 2019-07-31T23:39:15Z

Run RAT PreCommit

amaliujia · 2019-07-31T23:39:51Z

Run SQL PostCommit

amaliujia · 2019-08-01T00:36:19Z

Run SQL PostCommit

amaliujia · 2019-08-01T04:14:55Z

Run SQL PostCommit

amaliujia · 2019-08-01T04:18:39Z

Run Java PreCommit

amaliujia · 2019-08-01T05:10:11Z

Run SQL PostCommit

amaliujia · 2019-08-01T17:46:32Z

Run SQL PostCommit

amaliujia · 2019-08-01T17:51:54Z

Run SQL PostCommit

apilloud · 2019-08-01T19:50:29Z

sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java

@@ -309,6 +312,10 @@ private QueryPlanner instantiatePlanner(JdbcConnection jdbcConnection, RuleSet[]
        return new CalciteQueryPlanner(jdbcConnection, ruleSets);
      }

+      if (queryPlannerClassName.equals(ZETASQL_PLANNER)) {


Replace this with a reflection version and move it into its own PR:

try { return Class.forName(queryPlannerClassName) .getConstructor(QueryPlanner.class) .newInstance(jdbcConnection, ruleSets); } catch (Exception e) { throw new RuntimeException( String.format("Cannot construct query planner %s", queryPlannerClassName), e); }

apilloud · 2019-08-01T19:53:47Z

Please move the ZetaSQL code into its own package. "sdks/java/extensions/sql/zetasql" would be a good choice. Once that is done, you can send an email to dev@ proposing this as a code drop. If that proposal is accepted I'll give it a quick code review. If not, we can discuss breaking this up into smaller pieces.

amaliujia · 2019-08-02T03:59:09Z

@apilloud
During moving ZetaSQL planner to another package, I find a dependency problem: Calcite.

BeamSQL depends on Calcite and does relocation on it.
ZetaSQL planner also depends on Calcite.

The problem is, in order to initialize an instance of ZetaSQL planner, we expect BeamSQL pass in a config to ZetaSQL planner, which unfortunately is CalciteConnectionConfig. Clearly, it causes a conflict and code fails to compile.

The right solution is vendored Calcite, which is in progress: #9189

So I suggest we still keep all code in BeamSQL, but in the dedicated zetasql directory. Once vendored Calcite effort is done, we can move ZetaSQL code to a separate module.

What do you think? Do you have a better idea?

kanterov · 2019-08-02T09:15:23Z

I tried to run our internal test suite against this branch. There is a problem with gRPC dependency from jni_channel. It doesn't seem to be using vendored gRPC, or relocating gRPC classes properly.

kanterov · 2019-08-02T09:18:51Z

It was causing:

java.lang.ClassNotFoundException: io.grpc.StatusRuntimeException

It seems there is an issue with Netty as well:

java.lang.NoClassDefFoundError: io/netty/channel/socket/nio/NioSocketChannel
	at org.apache.beam.repackaged.sql.com.google.zetasql.io.grpc.netty.NettyChannelBuilder.<init>(NettyChannelBuilder.java:73)
	at org.apache.beam.repackaged.sql.com.google.zetasql.io.grpc.netty.NettyChannelBuilder.forAddress(NettyChannelBuilder.java:94)

amaliujia · 2019-08-02T22:30:54Z

During the process of moving zetasql planner to another package, I also experienced missing grpc dependency.

apilloud · 2019-08-02T22:57:00Z

The grpc issue might be a ZetaSQL bug, I'll work with you to get an isolated repro on Monday. As for moving this to a separate package, I would consider that a blocker to this merging. ZetaSQL currently only works on a limited subset of linux machines, we don't want to break other users and developers. I see #9189 is waiting for my review, so I'm going to do that now to help move this forward.

apilloud

This is too large to review, but it is a code drop so we are OK with that. I reviewed the changes outside of zetasql then skimmed the rest.

LGTM

apilloud · 2019-08-20T20:56:28Z

.test-infra/jenkins/job_PreCommit_BeamSQL_ZetaSQL.groovy

+PrecommitJobBuilder builder = new PrecommitJobBuilder(
+        scope: this,
+        nameBase: 'JavaBeamZetaSQL',
+        gradleTask: ':javaPreCommitBeamZetaSQL',


I'm not sure this justifies its own jenkins precommit. Should this just be added to the existing SQL precommit?

If you do plan to keep, please test that this works in the PR prior to merging.

There is no SQL precommit. There is a SQL postcommit. I am actually happy to add to an existing precommit if there is any suitable.

Currently the SQL unit tests run as part of the Java precommit.

apilloud · 2019-08-20T20:59:53Z

sdks/java/extensions/sql/build.gradle

@@ -35,6 +35,8 @@ applyJavaNature(
      include(dependency("org.apache.calcite:.*"))
      include(dependency("org.apache.calcite.avatica:.*"))
      include(dependency("org.codehaus.janino:.*"))
+      include(dependency("com.google.api.grpc:.*"))


This whole block of things goes away in #9189. We will probably need to vendor ZetaSQL. (No action required now, just want to make sure you are aware of the conflict.) cc: @vectorijk

apilloud · 2019-08-20T21:01:47Z

sdks/java/extensions/sql/build.gradle

@@ -82,6 +92,10 @@ dependencies {
  compile "org.apache.calcite:calcite-core:$calcite_version"
  compile "org.apache.calcite:calcite-linq4j:$calcite_version"
  compile "org.apache.calcite.avatica:avatica-core:$avatica_version"
+  compile "com.google.api.grpc:proto-google-common-protos:1.12.0" // Interfaces with ZetaSQL use this
+  compile "com.google.zetasql:zetasql-jni-channel:2019.07.1"


We should make the version 2019.07.1 a variable.

apilloud · 2019-08-20T21:02:40Z

sdks/java/extensions/sql/build.gradle

+  // Disable Gradle cache (it should not be used because the IT's won't run).
+  outputs.upToDateWhen { false }
+
+  include '**/*ZetaSQL.class'


Is it possible to make this *TestZetaSQL.class instead?

apilloud · 2019-08-20T21:03:21Z

sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/BeamSqlEnv.java

@@ -132,6 +132,8 @@ public String explain(String sqlString) throws ParseException {
  public static class BeamSqlEnvBuilder {
    private static final String CALCITE_PLANNER =
        "org.apache.beam.sdk.extensions.sql.impl.CalciteQueryPlanner";
+    private static final String ZETASQL_PLANNER =


This doesn't belong here. Can you just delete it?

apilloud · 2019-08-20T21:09:15Z

...sql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SqlStdOperatorMappingTable.java

+          FunctionSignatureId.FN_CASE_NO_VALUE,
+          FunctionSignatureId.FN_CASE_WITH_VALUE,
+          FunctionSignatureId.FN_TIMESTAMP_ADD,
+          // FunctionSignatureId.FN_TIMESTAMP_SUB,


There are a whole bunch of these commented out values, should they just be deleted? (I'm guessing they are effectively TODOs, but want to confirm.)

They are effectively TODO or partially supported. I will add comment to mark it.

amaliujia · 2019-08-20T21:34:17Z

Run javaPreCommitBeamZetaSQL

amaliujia · 2019-08-20T21:55:22Z

Run Seed Job

amaliujia · 2019-08-20T22:08:28Z

Run JavaBeamZetaSQL Precommit

amaliujia · 2019-08-20T23:22:48Z

Run Python PreCommit

kennknowles

I have left some comments. Since this is so huge, it is best to merge first and then fix the issues after, I think. The main thing that probably is new to this PR is the integration with Beam's gradle and Jenkins configurations. I think you should not have to do any new configuration to run your tests. Maybe I missed something special about them?

kennknowles · 2019-08-21T05:11:31Z

.test-infra/jenkins/job_PreCommit_BeamSQL_ZetaSQL.groovy

+PrecommitJobBuilder builder = new PrecommitJobBuilder(
+        scope: this,
+        nameBase: 'JavaBeamZetaSQL',
+        gradleTask: ':javaPreCommitBeamZetaSQL',


Currently the SQL unit tests run as part of the Java precommit.

kennknowles · 2019-08-21T05:13:57Z

sdks/java/extensions/sql/build.gradle

@@ -187,6 +208,19 @@ task runPojoExample(type: JavaExec) {
  args = ["--runner=DirectRunner"]
 }

+task runZetaSQLTest(type: Test) {
+  // Disable Gradle cache (it should not be used because the IT's won't run).


Where are these ITs? I was scanning the files but did not see them.

kennknowles · 2019-08-21T05:16:33Z

.../src/test/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLDialectSpecTestZetaSQL.java

+
+/** ZetaSQLDialectSpecTest. */
+@RunWith(JUnit4.class)
+public class ZetaSQLDialectSpecTestZetaSQL {


Normally this class would end with Test. It has a strange name. But more important is that people may not notice it if they are reading quickly for test classes, or using find command line tool, etc.

It is also far too big. Should be broken up into areas of the dialect to make it easier to navigate and control which tests execute.

I agree. Tests should be split.

For naming, ending Test will be matched as normal SQL tests (which we should not allow it happen due to less platform support of ZetaSQL). It is why IT is the end for integration tests. I followed the same convention to add ZetaSQL after Test to indicate what kind of tests it is.

apilloud · 2019-08-22T18:26:20Z

Looks like the SQL post commit didn't get run on this. It started hard failing immediately after this went in: https://issues.apache.org/jira/browse/BEAM-8036

apilloud mentioned this pull request Jul 31, 2019

How to use? google/zetasql#4

Open

amaliujia requested a review from apilloud August 1, 2019 19:08

apilloud reviewed Aug 1, 2019

View reviewed changes

[BEAM-7832] Add ZetaSQL as a dialect in BeamSQL.

e2fd047

amaliujia force-pushed the rw_beam_zetasql branch from 5adf530 to e2fd047 Compare August 12, 2019 19:46

amaliujia requested a review from kennknowles August 15, 2019 04:37

apilloud approved these changes Aug 20, 2019

View reviewed changes

fixup: address commnents

2df3b26

kennknowles approved these changes Aug 21, 2019

View reviewed changes

amaliujia merged commit 68734a2 into master Aug 21, 2019

amaliujia deleted the rw_beam_zetasql branch August 21, 2019 19:15

apilloud mentioned this pull request Oct 19, 2019

Test ZetaSQLPlanner in DataCatalogBigQueryIT #9828

Merged

3 tasks

[BEAM-7832] Add ZetaSQL as a dialect in BeamSQL #9210

[BEAM-7832] Add ZetaSQL as a dialect in BeamSQL #9210

Conversation

amaliujia commented Jul 31, 2019 • edited Loading

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

amaliujia commented Jul 31, 2019

amaliujia commented Jul 31, 2019

amaliujia commented Jul 31, 2019

amaliujia commented Aug 1, 2019

amaliujia commented Aug 1, 2019

amaliujia commented Aug 1, 2019

amaliujia commented Aug 1, 2019

amaliujia commented Aug 1, 2019

amaliujia commented Aug 1, 2019

Choose a reason for hiding this comment

apilloud commented Aug 1, 2019

amaliujia commented Aug 2, 2019 • edited Loading

kanterov commented Aug 2, 2019

kanterov commented Aug 2, 2019 • edited Loading

amaliujia commented Aug 2, 2019

apilloud commented Aug 2, 2019

apilloud 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

Choose a reason for hiding this comment

apilloud Aug 20, 2019 • 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

amaliujia commented Aug 20, 2019

amaliujia commented Aug 20, 2019

amaliujia commented Aug 20, 2019

amaliujia commented Aug 20, 2019

kennknowles 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

apilloud commented Aug 22, 2019

amaliujia commented Jul 31, 2019 •

edited

Loading

amaliujia commented Aug 2, 2019 •

edited

Loading

kanterov commented Aug 2, 2019 •

edited

Loading

apilloud Aug 20, 2019 •

edited

Loading