[BEAM-8592] Adjusting ZetaSQL table resolution to standard #10021

kennknowles · 2019-11-07T03:18:01Z

Because I am developing on Mac and ZetaSQL does not support Mac I cannot run the required unit tests so easily. Uploading to run them.

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.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

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.

kennknowles · 2019-11-07T04:15:01Z

run sql postcommit

kennknowles · 2019-11-07T04:15:37Z

Amazingly, since I can't run the tests locally, the ZetaSQL precommit passed.

kennknowles · 2019-11-07T04:16:28Z

I'm expecting the data catalog postcommit to fail.

kennknowles · 2019-11-07T04:20:35Z

s/amazingly/suspiciously/g

Investigating the gradle scans

kennknowles · 2019-11-08T01:58:40Z

run sql postcommit

kennknowles · 2019-11-08T02:15:56Z

OK, confirmed that it works at least for the test coverage we have. Pushing a tiny cleanup. I'm on Mac now so I cannot confirm it so quickly.

kennknowles · 2019-11-08T02:29:05Z

run sql postcommit

kennknowles · 2019-11-08T04:04:26Z

run sql precommit

amaliujia

My only question is, as JoinCompoundIdentifiersTest.java will be deleted, seems like we will lose all unit tests on table resolution?

kennknowles · 2019-11-08T20:34:05Z

"join compound identifiers" table resolver is removed. In fact, all uses of TableResolver are unused. There is just one form of table resolution. There is a lot of unused code that I did not get all the way to deleting because I was focused on getting it working first. I will now go and clear it up a bit more.

I will take another look at the test file and see if there are tests that should be in unit tests of some other class.

kennknowles · 2019-11-08T20:36:58Z

I took another look and I think all the functionality tested by that file is gone.

amaliujia · 2019-11-09T00:56:43Z

Let me ask in another way: do we need unit tests for table resolution? If so, can you add some?

kennknowles · 2019-11-10T19:50:00Z

Thanks for clarifying. That makes sense. I will review the existing tests and make sure we have coverage of a few different situations. For example I noticed that SqlTransform.withDefaultTableProvider is different than how the unit tests are set up.

kennknowles · 2019-11-15T05:14:40Z

Ah. Now that I have looked closer, these tests were running against ReadOnlyTableProvider which does not support real table resolution. So it used join components to just create a string for lookup.

Since ZetaSQL table reference (list of strings) is the same structure as a Calcite table reference (list of strings), the main thing that is helpful is just unit testing of the one real method of TableResolution. The only real logic there is doing a for loop over that list of strings. I will write a couple of tests for that.

kennknowles · 2019-11-18T22:32:21Z

run sql postcommit

kennknowles · 2019-11-20T23:58:25Z

Please take another look. I have restored unit testing to ensure that TableResolution works as expected.

apilloud

LGTM.

Might be worth adding a test for tables with dot in their name.

apilloud · 2019-11-21T18:35:11Z

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

-      return TableResolver.DEFAULT_ASSUME_LEAF_IS_TABLE;
+  // Set up table providers that need to be pre-registered
+  // TODO: share this logic between dialects, set up tables on JDBC connection too
+


nit:Extra whitespace?

Done & filed a Jira for this TODO

apilloud · 2019-11-21T18:36:00Z

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

-   * the getSubschema() path until the second-to-last path element. We expect the path to be a table
-   * path, so the last element should be a valid table id, we don't expect anything else there.
-   *
-   * <p>This resembles a default Calcite planner strategy. One difference is that Calcite doesn't


A bit of this comment might still be useful in explaining what this is doing.

apilloud · 2019-11-21T18:36:58Z

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

@@ -134,6 +136,11 @@ public RelRoot rel(String sql, Map<String, Value> params) {

    QueryTrait trait = new QueryTrait();

+    // Set up table providers that need to be pre-registered
+    // TODO: share this logic between dialects, set up tables on JDBC connection too


Nit: JIRA for the todo?

kennknowles

Good point about dots in names. Added tests for those in table name and also schema. It only tests the table resolution logic, not the parser. I feel it is probably not our job to test the parser.

kennknowles · 2019-11-24T02:58:03Z

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

-   * the getSubschema() path until the second-to-last path element. We expect the path to be a table
-   * path, so the last element should be a valid table id, we don't expect anything else there.
-   *
-   * <p>This resembles a default Calcite planner strategy. One difference is that Calcite doesn't


kennknowles · 2019-11-24T03:04:13Z

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

-      return TableResolver.DEFAULT_ASSUME_LEAF_IS_TABLE;
+  // Set up table providers that need to be pre-registered
+  // TODO: share this logic between dialects, set up tables on JDBC connection too
+


Done & filed a Jira for this TODO

kennknowles · 2019-11-24T03:06:20Z

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

@@ -134,6 +136,11 @@ public RelRoot rel(String sql, Map<String, Value> params) {

    QueryTrait trait = new QueryTrait();

+    // Set up table providers that need to be pre-registered
+    // TODO: share this logic between dialects, set up tables on JDBC connection too


kennknowles · 2019-11-24T03:36:10Z

run sql postcommit

kennknowles changed the title ~~WIP adjusting ZetaSQL table resolution to standard~~ Adjusting ZetaSQL table resolution to standard Nov 7, 2019

kennknowles requested review from amaliujia and apilloud November 7, 2019 04:16

kennknowles force-pushed the TableResolverImpl branch from ad8d35c to 983917f Compare November 7, 2019 22:37

kennknowles force-pushed the TableResolverImpl branch 2 times, most recently from 6f47280 to 5733ce9 Compare November 8, 2019 02:28

kennknowles changed the title ~~Adjusting ZetaSQL table resolution to standard~~ [BEAM-8592] Adjusting ZetaSQL table resolution to standard Nov 8, 2019

amaliujia reviewed Nov 8, 2019

View reviewed changes

kennknowles force-pushed the TableResolverImpl branch from 5733ce9 to 1d40671 Compare November 8, 2019 20:35

kennknowles force-pushed the TableResolverImpl branch from 1d40671 to 9869aa2 Compare November 15, 2019 03:57

kennknowles force-pushed the TableResolverImpl branch 3 times, most recently from 0b300b1 to e42343b Compare November 18, 2019 21:06

apilloud approved these changes Nov 21, 2019

View reviewed changes

Use only standard table resolution for ZetaSQL dialect

671b02a

kennknowles force-pushed the TableResolverImpl branch from e42343b to 671b02a Compare November 24, 2019 03:28

kennknowles commented Nov 24, 2019

View reviewed changes

kennknowles merged commit 639fc24 into apache:master Nov 24, 2019

kennknowles deleted the TableResolverImpl branch November 24, 2019 03:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-8592] Adjusting ZetaSQL table resolution to standard #10021

[BEAM-8592] Adjusting ZetaSQL table resolution to standard #10021

kennknowles commented Nov 7, 2019 •

edited

Loading

kennknowles commented Nov 7, 2019

kennknowles commented Nov 7, 2019

kennknowles commented Nov 7, 2019

kennknowles commented Nov 7, 2019

kennknowles commented Nov 8, 2019

kennknowles commented Nov 8, 2019

kennknowles commented Nov 8, 2019

kennknowles commented Nov 8, 2019

amaliujia left a comment •

edited

Loading

kennknowles commented Nov 8, 2019

kennknowles commented Nov 8, 2019

amaliujia commented Nov 9, 2019

kennknowles commented Nov 10, 2019

kennknowles commented Nov 15, 2019

kennknowles commented Nov 18, 2019

kennknowles commented Nov 20, 2019

apilloud left a comment

apilloud Nov 21, 2019

kennknowles Nov 24, 2019

apilloud Nov 21, 2019

kennknowles Nov 24, 2019

apilloud Nov 21, 2019

kennknowles Nov 24, 2019

kennknowles left a comment

kennknowles Nov 24, 2019

kennknowles Nov 24, 2019

kennknowles Nov 24, 2019

kennknowles commented Nov 24, 2019

[BEAM-8592] Adjusting ZetaSQL table resolution to standard #10021

[BEAM-8592] Adjusting ZetaSQL table resolution to standard #10021

Conversation

kennknowles commented Nov 7, 2019 • edited Loading

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

kennknowles commented Nov 7, 2019

kennknowles commented Nov 7, 2019

kennknowles commented Nov 7, 2019

kennknowles commented Nov 7, 2019

kennknowles commented Nov 8, 2019

kennknowles commented Nov 8, 2019

kennknowles commented Nov 8, 2019

kennknowles commented Nov 8, 2019

amaliujia left a comment • edited Loading

Choose a reason for hiding this comment

kennknowles commented Nov 8, 2019

kennknowles commented Nov 8, 2019

amaliujia commented Nov 9, 2019

kennknowles commented Nov 10, 2019

kennknowles commented Nov 15, 2019

kennknowles commented Nov 18, 2019

kennknowles commented Nov 20, 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

Choose a reason for hiding this comment

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

kennknowles commented Nov 24, 2019

kennknowles commented Nov 7, 2019 •

edited

Loading

amaliujia left a comment •

edited

Loading