You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/07/03 00:40:43 UTC
[GitHub] [beam] amaliujia opened a new pull request #12169: [SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
amaliujia opened a new pull request #12169:
URL: https://github.com/apache/beam/pull/12169
This PR adds the support of
```
"CREATE TABLE FUNCTION CustomerRange(MinID INT64, MaxID INT64)\n"
" AS\n"
" SELECT *\n"
" FROM KeyValue\n"
" WHERE key >= MinId AND key <= MaxId; \n"
" SELECT key FROM CustomerRange(10, 14)";
```
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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.
- [ ] Update `CHANGES.md` with noteworthy changes.
- [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450522705
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -171,6 +178,18 @@ public RelRoot rel(String sql, QueryParameters params) {
SqlAnalyzer.USER_DEFINED_FUNCTIONS,
String.join(".", createFunctionStmt.getNamePath()));
udfBuilder.put(functionFullName, createFunctionStmt);
+ } else if (statement.nodeKind() == RESOLVED_CREATE_TABLE_FUNCTION_STMT) {
+ ResolvedCreateTableFunctionStmt createTableFunctionStmt =
+ (ResolvedCreateTableFunctionStmt) statement;
+ catalog.addTableValuedFunction(
Review comment:
Sorry I am getting an action to address this comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450518775
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/ZetaSqlUserDefinedPureSqlTableValuedFunction.java
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.sql.impl;
+
+import java.util.List;
+import org.apache.beam.sdk.annotations.Internal;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.rel.type.RelDataType;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.schema.Function;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.SqlIdentifier;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.type.SqlOperandTypeInference;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.validate.SqlUserDefinedFunction;
+
+/** This is a class to indicate that a TVF is a ZetaSQL pure SQL UDTVF. */
+@Internal
+public class ZetaSqlUserDefinedPureSqlTableValuedFunction extends SqlUserDefinedFunction {
Review comment:
`PureSql` is used to differentiate `Java` and `Python` UDTVF, because right now I think the latter will hit different path.
If in the future they all turn into use the same path, I will change this class by removing `PureSql`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450616651
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -211,7 +212,7 @@ public RelRoot rel(String sql, QueryParameters params) {
RelNode convertedNode =
QueryStatementConverter.convertRootQuery(context, (ResolvedQueryStmt) statement);
-
+ LOG.info("SQLPlan>\n" + RelOptUtil.toString(convertedNode));
Review comment:
oops. This is a line that was not added intentionally. Have removed it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450526675
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -211,7 +212,7 @@ public RelRoot rel(String sql, QueryParameters params) {
RelNode convertedNode =
QueryStatementConverter.convertRootQuery(context, (ResolvedQueryStmt) statement);
-
+ LOG.info("SQLPlan>\n" + RelOptUtil.toString(convertedNode));
Review comment:
This should probably be `debug` level.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #12169:
URL: https://github.com/apache/beam/pull/12169#issuecomment-654506366
I have added more comments to function arguments in ConversionContext. I hope it could clarify the purpose if this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on pull request #12169: [SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #12169:
URL: https://github.com/apache/beam/pull/12169#issuecomment-653274247
R: @ibzib
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia edited a comment on pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #12169:
URL: https://github.com/apache/beam/pull/12169#issuecomment-654506366
I have added more comments to function arguments in ConversionContext. I hope it could clarify the purpose of this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia merged pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia merged pull request #12169:
URL: https://github.com/apache/beam/pull/12169
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450529634
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ConversionContext.java
##########
@@ -33,7 +33,16 @@
private final ExpressionConverter expressionConverter;
private final RelOptCluster cluster;
private final QueryTrait trait;
- private final Map<List<String>, ResolvedNode> sqlUDTVF;
+ // Pure-sql user-defined table-valued function can be resolved by Analyzer. Keeping the
Review comment:
The comments here are all about "pure-sql" UDF (it won't apply to java/python UDF). Is there a better naming in your mind (or you are prefer to add a link some where to explain what is "pure-sql" UDF?)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450517449
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/FilterScanConverter.java
##########
@@ -47,7 +46,7 @@ public RelNode convert(ResolvedFilterScan zetaNode, List<RelNode> inputs) {
zetaNode.getFilterExpr(),
zetaNode.getInputScan().getColumnList(),
input.getRowType().getFieldList(),
- ImmutableMap.of());
+ context.getFunctionArgumentRefMapping());
Review comment:
I am planning to improve testing coverage in next steps and gradually make necessary converters to use `getFunctionArgumentRefMapping`. E.g. in project or join.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450520694
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -171,6 +178,18 @@ public RelRoot rel(String sql, QueryParameters params) {
SqlAnalyzer.USER_DEFINED_FUNCTIONS,
String.join(".", createFunctionStmt.getNamePath()));
Review comment:
Yeah, I will do that as a follow-up to avoid merge conflicts.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450436253
##########
File path: sdks/java/extensions/sql/zetasql/src/test/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLDialectSpecTest.java
##########
@@ -2916,6 +2917,57 @@ public void testRecursiveUdfThrowsException() {
zetaSQLQueryPlanner.convertToBeamRel(sql);
}
+ @Test
+ public void testUDTVF() {
+ String sql =
+ "CREATE TABLE FUNCTION CustomerRange(MinID INT64, MaxID INT64)\n"
+ + " AS\n"
+ + " SELECT *\n"
+ + " FROM KeyValue\n"
+ + " WHERE key >= MinId AND key <= MaxId; \n"
+ + " SELECT key FROM CustomerRange(10, 14)";
+
+ ZetaSQLQueryPlanner zetaSQLQueryPlanner = new ZetaSQLQueryPlanner(config);
+ BeamRelNode beamRelNode = zetaSQLQueryPlanner.convertToBeamRel(sql);
+ PCollection<Row> stream = BeamSqlRelUtils.toPCollection(pipeline, beamRelNode);
+
+ Schema singleField = Schema.builder().addInt64Field("field1").build();
+ PAssert.that(stream).containsInAnyOrder(Row.withSchema(singleField).addValues(14L).build());
+ pipeline.run().waitUntilFinish(Duration.standardMinutes(PIPELINE_EXECUTION_WAITTIME_MINUTES));
+ }
+
+ @Test
+ public void testUDTVFTableNotFound() {
+ String sql =
+ "CREATE TABLE FUNCTION CustomerRange(MinID INT64, MaxID INT64)\n"
+ + " AS\n"
+ + " SELECT *\n"
+ + " FROM TableNotExist\n"
+ + " WHERE key >= MinId AND key <= MaxId; \n"
+ + " SELECT key FROM CustomerRange(10, 14)";
+
+ ZetaSQLQueryPlanner zetaSQLQueryPlanner = new ZetaSQLQueryPlanner(config);
+ thrown.expect(SqlConversionException.class);
+ thrown.expectMessage("Wasn't able to find resolve the path [TableNotExist] in beam");
Review comment:
A couple nits about this error message:
- Remove "find."
- Instead of just "in beam" it should be "in *schema* beam" so it's clear what the name is referring to.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/TVFScanConverter.java
##########
@@ -42,45 +42,57 @@
@Override
public RelNode convert(ResolvedTVFScan zetaNode, List<RelNode> inputs) {
RelNode input = inputs.get(0);
+ RexCall call =
+ getExpressionConverter()
+ .convertTableValuedFunction(
+ input,
+ zetaNode.getTvf(),
+ zetaNode.getArgumentList(),
+ zetaNode.getArgumentList().get(0).getScan() != null
+ ? zetaNode.getArgumentList().get(0).getScan().getColumnList()
+ : Collections.emptyList());
RelNode tableFunctionScan =
LogicalTableFunctionScan.create(
- getCluster(),
- inputs,
- getExpressionConverter()
- .convertTableValuedFunction(
- input,
- zetaNode.getTvf(),
- zetaNode.getArgumentList(),
- zetaNode.getArgumentList().get(0).getScan().getColumnList()),
- null,
- createRowTypeWithWindowStartAndEnd(input.getRowType()),
- Collections.EMPTY_SET);
+ getCluster(), inputs, call, null, call.getType(), Collections.EMPTY_SET);
+ // Pure SQL UDF's language body is built bottom up, so FunctionArgumentRefMapping should be
+ // already consumed thus it can be cleared now.
+ context.clearFunctionArgumentRefMapping();
Review comment:
Why do we need to clear?
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SqlAnalyzer.java
##########
@@ -177,12 +179,17 @@ static AnalyzerOptions initAnalyzerOptions() {
LanguageFeature.FEATURE_DISALLOW_GROUP_BY_FLOAT,
LanguageFeature.FEATURE_V_1_2_CIVIL_TIME,
LanguageFeature.FEATURE_V_1_1_SELECT_STAR_EXCEPT_REPLACE,
- LanguageFeature.FEATURE_TABLE_VALUED_FUNCTIONS)));
+ LanguageFeature.FEATURE_TABLE_VALUED_FUNCTIONS,
+ LanguageFeature.FEATURE_CREATE_TABLE_FUNCTION,
+ LanguageFeature.FEATURE_TEMPLATE_FUNCTIONS)));
options
.getLanguageOptions()
.setSupportedStatementKinds(
- ImmutableSet.of(RESOLVED_QUERY_STMT, RESOLVED_CREATE_FUNCTION_STMT));
+ ImmutableSet.of(
Review comment:
We should make this a constant so we can reference it above.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SqlAnalyzer.java
##########
@@ -156,7 +157,8 @@ ResolvedStatement analyzeNextStatement(
"Failed to define function %s", String.join(".", createFunctionStmt.getNamePath())),
e);
}
- } else if (resolvedStatement.nodeKind() != RESOLVED_QUERY_STMT) {
+ } else if (resolvedStatement.nodeKind() != RESOLVED_QUERY_STMT
Review comment:
We should check set membership instead to make this more readable/scalable.
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/ZetaSqlUserDefinedPureSqlTableValuedFunction.java
##########
@@ -0,0 +1,49 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.beam.sdk.extensions.sql.impl;
+
+import java.util.List;
+import org.apache.beam.sdk.annotations.Internal;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.rel.type.RelDataType;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.schema.Function;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.SqlIdentifier;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.type.SqlOperandTypeChecker;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.type.SqlOperandTypeInference;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.type.SqlReturnTypeInference;
+import org.apache.beam.vendor.calcite.v1_20_0.org.apache.calcite.sql.validate.SqlUserDefinedFunction;
+
+/** This is a class to indicate that a TVF is a ZetaSQL pure SQL UDTVF. */
+@Internal
+public class ZetaSqlUserDefinedPureSqlTableValuedFunction extends SqlUserDefinedFunction {
Review comment:
Nit: `PureSql` is probably not necessary, since we already have `ZetaSql` in the class name.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ConversionContext.java
##########
@@ -27,24 +33,39 @@
private final ExpressionConverter expressionConverter;
private final RelOptCluster cluster;
private final QueryTrait trait;
+ private final Map<List<String>, ResolvedNode> sqlUDTVF;
Review comment:
Please avoid abbreviation here. I recommend `userDefinedTableValuedFunctions` (`sql` is probably redundant in this case).
Also consider adding an explanatory comment.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ConversionContext.java
##########
@@ -62,4 +83,20 @@ RelOptCluster cluster() {
QueryTrait getTrait() {
return trait;
}
+
+ Map<List<String>, ResolvedNode> getSqlUDTVF() {
+ return sqlUDTVF;
+ }
+
+ Map<String, RexNode> getFunctionArgumentRefMapping() {
+ return functionArgumentRefMapping;
+ }
+
+ void addToFunctionArgumentRefMapping(String s, RexNode r) {
Review comment:
Optional nit: `addToFunctionArgumentRefMapping` and `clearFunctionArgumentRefMapping` can be replaced by `getFunctionArgumentRefMapping().put()` and `getFunctionArgumentRefMapping().clear()`.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -171,6 +178,18 @@ public RelRoot rel(String sql, QueryParameters params) {
SqlAnalyzer.USER_DEFINED_FUNCTIONS,
String.join(".", createFunctionStmt.getNamePath()));
udfBuilder.put(functionFullName, createFunctionStmt);
+ } else if (statement.nodeKind() == RESOLVED_CREATE_TABLE_FUNCTION_STMT) {
+ ResolvedCreateTableFunctionStmt createTableFunctionStmt =
+ (ResolvedCreateTableFunctionStmt) statement;
+ catalog.addTableValuedFunction(
Review comment:
We add UDF to the catalog in SqlAnalyzer. It's probably fine to add to the catalog in either SqlAnalyzer or ZetaSQLPlannerImpl, but please keep it consistent.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -171,6 +178,18 @@ public RelRoot rel(String sql, QueryParameters params) {
SqlAnalyzer.USER_DEFINED_FUNCTIONS,
String.join(".", createFunctionStmt.getNamePath()));
Review comment:
I suppose I should have used getNamePath instead of getFullName...
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/FilterScanConverter.java
##########
@@ -47,7 +46,7 @@ public RelNode convert(ResolvedFilterScan zetaNode, List<RelNode> inputs) {
zetaNode.getFilterExpr(),
zetaNode.getInputScan().getColumnList(),
input.getRowType().getFieldList(),
- ImmutableMap.of());
+ context.getFunctionArgumentRefMapping());
Review comment:
Should we should change all converters to use `getFunctionArgumentRefMapping`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450527335
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ConversionContext.java
##########
@@ -33,7 +33,16 @@
private final ExpressionConverter expressionConverter;
private final RelOptCluster cluster;
private final QueryTrait trait;
- private final Map<List<String>, ResolvedNode> sqlUDTVF;
+ // Pure-sql user-defined table-valued function can be resolved by Analyzer. Keeping the
Review comment:
I'm guessing the meaning of `Pure-sql` might not be clear to those who are unfamiliar with our plans to add cross-language UDF.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450526186
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -171,6 +178,18 @@ public RelRoot rel(String sql, QueryParameters params) {
SqlAnalyzer.USER_DEFINED_FUNCTIONS,
String.join(".", createFunctionStmt.getNamePath()));
udfBuilder.put(functionFullName, createFunctionStmt);
+ } else if (statement.nodeKind() == RESOLVED_CREATE_TABLE_FUNCTION_STMT) {
+ ResolvedCreateTableFunctionStmt createTableFunctionStmt =
+ (ResolvedCreateTableFunctionStmt) statement;
+ catalog.addTableValuedFunction(
Review comment:
https://github.com/apache/beam/blob/103facbf80c2e4786e60367161ec59352196d04a/sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SqlAnalyzer.java#L152
I suggest adding scalar functions and TVF to the catalog in the same file. So either move `catalog.add(scalar function)` to ZetaSQLPlannerImpl or move `catalog.add(TVF)` to SqlAnalyzer.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450517162
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/TVFScanConverter.java
##########
@@ -42,45 +42,57 @@
@Override
public RelNode convert(ResolvedTVFScan zetaNode, List<RelNode> inputs) {
RelNode input = inputs.get(0);
+ RexCall call =
+ getExpressionConverter()
+ .convertTableValuedFunction(
+ input,
+ zetaNode.getTvf(),
+ zetaNode.getArgumentList(),
+ zetaNode.getArgumentList().get(0).getScan() != null
+ ? zetaNode.getArgumentList().get(0).getScan().getColumnList()
+ : Collections.emptyList());
RelNode tableFunctionScan =
LogicalTableFunctionScan.create(
- getCluster(),
- inputs,
- getExpressionConverter()
- .convertTableValuedFunction(
- input,
- zetaNode.getTvf(),
- zetaNode.getArgumentList(),
- zetaNode.getArgumentList().get(0).getScan().getColumnList()),
- null,
- createRowTypeWithWindowStartAndEnd(input.getRowType()),
- Collections.EMPTY_SET);
+ getCluster(), inputs, call, null, call.getType(), Collections.EMPTY_SET);
+ // Pure SQL UDF's language body is built bottom up, so FunctionArgumentRefMapping should be
+ // already consumed thus it can be cleared now.
+ context.clearFunctionArgumentRefMapping();
Review comment:
The SQL plan building called from top, but built from bottom.
In this case, TVFScanConverter will be called first, where it knows what values of function parameters used in later SELECT, so we add those parameters to Context. Then it goes down to inputs of TVFScanConverter. For pure SQL UDTVF, per definition, inputs of TVFScanConverter are UDTVF's SQL language body, which are also ResolvedNode, and FuncitonArugmentRefMapping is consumed. After the conversion goes back to TVFScanConverter, which means its input has finished conversion (thus has consumed FuncitonArugmentRefMapping), thus `FuncitonArugmentRefMapping` can be cleared.
The current implementation in this PR is still fragile. For example, if there are more than one TVFScanConverter, I am thinking current implementation will break. So my nest step is to improve testing coverage and catch more complicated cases.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450527691
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ConversionContext.java
##########
@@ -62,4 +83,20 @@ RelOptCluster cluster() {
QueryTrait getTrait() {
return trait;
}
+
+ Map<List<String>, ResolvedNode> getSqlUDTVF() {
+ return sqlUDTVF;
+ }
+
+ Map<String, RexNode> getFunctionArgumentRefMapping() {
+ return functionArgumentRefMapping;
+ }
+
+ void addToFunctionArgumentRefMapping(String s, RexNode r) {
Review comment:
I was suggesting removing `addToFunctionArgumentRefMapping` and `clearFunctionArgumentRefMapping` completely, and using only `getFunctionArgumentRefMapping()` in other classes. But it's up to you.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450616472
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ConversionContext.java
##########
@@ -33,7 +33,16 @@
private final ExpressionConverter expressionConverter;
private final RelOptCluster cluster;
private final QueryTrait trait;
- private final Map<List<String>, ResolvedNode> sqlUDTVF;
+ // Pure-sql user-defined table-valued function can be resolved by Analyzer. Keeping the
Review comment:
Adopted "SQL native".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450553087
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ConversionContext.java
##########
@@ -33,7 +33,16 @@
private final ExpressionConverter expressionConverter;
private final RelOptCluster cluster;
private final QueryTrait trait;
- private final Map<List<String>, ResolvedNode> sqlUDTVF;
+ // Pure-sql user-defined table-valued function can be resolved by Analyzer. Keeping the
Review comment:
Perhaps "native SQL"/"SQL native" functions is a more apt terminology.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450518142
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/TVFScanConverter.java
##########
@@ -42,45 +42,57 @@
@Override
public RelNode convert(ResolvedTVFScan zetaNode, List<RelNode> inputs) {
RelNode input = inputs.get(0);
+ RexCall call =
+ getExpressionConverter()
+ .convertTableValuedFunction(
+ input,
+ zetaNode.getTvf(),
+ zetaNode.getArgumentList(),
+ zetaNode.getArgumentList().get(0).getScan() != null
+ ? zetaNode.getArgumentList().get(0).getScan().getColumnList()
+ : Collections.emptyList());
RelNode tableFunctionScan =
LogicalTableFunctionScan.create(
- getCluster(),
- inputs,
- getExpressionConverter()
- .convertTableValuedFunction(
- input,
- zetaNode.getTvf(),
- zetaNode.getArgumentList(),
- zetaNode.getArgumentList().get(0).getScan().getColumnList()),
- null,
- createRowTypeWithWindowStartAndEnd(input.getRowType()),
- Collections.EMPTY_SET);
+ getCluster(), inputs, call, null, call.getType(), Collections.EMPTY_SET);
+ // Pure SQL UDF's language body is built bottom up, so FunctionArgumentRefMapping should be
+ // already consumed thus it can be cleared now.
+ context.clearFunctionArgumentRefMapping();
Review comment:
There are a lot more to test, including JOIN and AGGREGATE in UDTVF.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450522705
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -171,6 +178,18 @@ public RelRoot rel(String sql, QueryParameters params) {
SqlAnalyzer.USER_DEFINED_FUNCTIONS,
String.join(".", createFunctionStmt.getNamePath()));
udfBuilder.put(functionFullName, createFunctionStmt);
+ } else if (statement.nodeKind() == RESOLVED_CREATE_TABLE_FUNCTION_STMT) {
+ ResolvedCreateTableFunctionStmt createTableFunctionStmt =
+ (ResolvedCreateTableFunctionStmt) statement;
+ catalog.addTableValuedFunction(
Review comment:
Sorry I am not getting an action to address this comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450616694
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -171,6 +178,18 @@ public RelRoot rel(String sql, QueryParameters params) {
SqlAnalyzer.USER_DEFINED_FUNCTIONS,
String.join(".", createFunctionStmt.getNamePath()));
udfBuilder.put(functionFullName, createFunctionStmt);
+ } else if (statement.nodeKind() == RESOLVED_CREATE_TABLE_FUNCTION_STMT) {
+ ResolvedCreateTableFunctionStmt createTableFunctionStmt =
+ (ResolvedCreateTableFunctionStmt) statement;
+ catalog.addTableValuedFunction(
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] amaliujia commented on a change in pull request #12169: [BEAM-9953[SQL][ZetaSQL] Support Pure SQL user-defined table-valued function.
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #12169:
URL: https://github.com/apache/beam/pull/12169#discussion_r450520293
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLPlannerImpl.java
##########
@@ -171,6 +178,18 @@ public RelRoot rel(String sql, QueryParameters params) {
SqlAnalyzer.USER_DEFINED_FUNCTIONS,
String.join(".", createFunctionStmt.getNamePath()));
Review comment:
Ah you mean use a List<> than joined name? I think they are equivalent but have a common way to use function name is a good idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org