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 2021/03/15 22:23:21 UTC
[GitHub] [beam] ibzib opened a new pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
ibzib opened a new pull request #14241:
URL: https://github.com/apache/beam/pull/14241
All remaining internal compliance test errors and crashes were caused by either BIGINT or DATE.
R: @apilloud
------------------------
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_VR_Dataflow_V2/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://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/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_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_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_Direct/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Dataflow/lastCompletedBuild/) | [](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 | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
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/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_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.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
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 pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #14241:
URL: https://github.com/apache/beam/pull/14241#issuecomment-799804041
> With this approach, new types are accepted by default. What is the advantage of rejecting unsupported types rather than allowing supported types?
I changed 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 #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r605309390
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamJavaUdfCalcRule.java
##########
@@ -52,4 +117,57 @@ public RelNode convert(RelNode rel) {
RelOptRule.convert(input, input.getTraitSet().replace(BeamLogicalConvention.INSTANCE)),
calc.getProgram());
}
+
+ /**
+ * Returns true only if the literal can be correctly implemented by {@link
+ * org.apache.beam.sdk.extensions.sql.impl.rel.BeamCalcRel} in ZetaSQL.
+ */
+ private static boolean udfSupportsLiteral(RexLiteral rexLiteral) {
+ switch (rexLiteral.getType().getSqlTypeName()) {
+ case BIGINT:
+ case BOOLEAN:
+ case DECIMAL:
+ case DOUBLE:
+ case TIME:
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case VARBINARY:
Review comment:
Oops, missed those. 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] apilloud commented on a change in pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
apilloud commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r605243389
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamJavaUdfCalcRule.java
##########
@@ -36,9 +47,63 @@ private BeamJavaUdfCalcRule() {
LogicalCalc.class, Convention.NONE, BeamLogicalConvention.INSTANCE, "BeamJavaUdfCalcRule");
}
+ /**
+ * Returns true if all the following are true: All RexCalls can be implemented by codegen, All
+ * RexCalls only contain ZetaSQL user-defined Java functions, All RexLiterals pass ZetaSQL
+ * compliance tests, All RexInputRefs pass ZetaSQL compliance tests, No other RexNode types
+ * Otherwise returns false. ZetaSQL user-defined Java functions are in the category whose function
+ * group is equal to {@code SqlAnalyzer.USER_DEFINED_JAVA_SCALAR_FUNCTIONS}
+ */
@Override
public boolean matches(RelOptRuleCall x) {
- return ZetaSQLQueryPlanner.hasOnlyJavaUdfInProjects(x);
Review comment:
Thank you for moving this!
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamJavaUdfCalcRule.java
##########
@@ -52,4 +117,57 @@ public RelNode convert(RelNode rel) {
RelOptRule.convert(input, input.getTraitSet().replace(BeamLogicalConvention.INSTANCE)),
calc.getProgram());
}
+
+ /**
+ * Returns true only if the literal can be correctly implemented by {@link
+ * org.apache.beam.sdk.extensions.sql.impl.rel.BeamCalcRel} in ZetaSQL.
+ */
+ private static boolean udfSupportsLiteral(RexLiteral rexLiteral) {
+ switch (rexLiteral.getType().getSqlTypeName()) {
+ case BIGINT:
+ case BOOLEAN:
+ case DECIMAL:
+ case DOUBLE:
+ case TIME:
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case VARBINARY:
+ case VARCHAR:
+ return true;
+ case DATE: // BEAM-11990
+ default:
+ return false;
+ }
+ }
+
+ /**
+ * Returns true only if the input ref can be correctly implemented by {@link
+ * org.apache.beam.sdk.extensions.sql.impl.rel.BeamCalcRel} in ZetaSQL.
+ */
+ private static boolean udfSupportsInput(RexInputRef rexInputRef) {
Review comment:
nit: I'm not sure this function adds value, consider just calling `udfSupportsInputType` directly?
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamJavaUdfCalcRule.java
##########
@@ -52,4 +117,57 @@ public RelNode convert(RelNode rel) {
RelOptRule.convert(input, input.getTraitSet().replace(BeamLogicalConvention.INSTANCE)),
calc.getProgram());
}
+
+ /**
+ * Returns true only if the literal can be correctly implemented by {@link
+ * org.apache.beam.sdk.extensions.sql.impl.rel.BeamCalcRel} in ZetaSQL.
+ */
+ private static boolean udfSupportsLiteral(RexLiteral rexLiteral) {
+ switch (rexLiteral.getType().getSqlTypeName()) {
+ case BIGINT:
+ case BOOLEAN:
+ case DECIMAL:
+ case DOUBLE:
+ case TIME:
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case VARBINARY:
Review comment:
I think we had a previous discussion on this CL about `CHAR`, `BINARY`, and `NULL`... They are all missing here.
--
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 pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #14241:
URL: https://github.com/apache/beam/pull/14241#issuecomment-812684072
Finally got this to pass compliance tests.
--
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] apilloud commented on a change in pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
apilloud commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r597298103
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
Review comment:
I was under the impression that null string should be treated as `NULL`, but I might be wrong here.
--
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 pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib commented on pull request #14241:
URL: https://github.com/apache/beam/pull/14241#issuecomment-810583173
@apilloud I addressed your comments. I also did some refactoring because the type matching for the rules should belong in the rules themselves. PTAL
--
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 merged pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib merged pull request #14241:
URL: https://github.com/apache/beam/pull/14241
--
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 #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r597293090
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
Review comment:
I was doing some debugging and found that Calcite treats a null string as `VARCHAR`. So we'll need to include both.
--
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 #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r605309699
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamJavaUdfCalcRule.java
##########
@@ -52,4 +117,57 @@ public RelNode convert(RelNode rel) {
RelOptRule.convert(input, input.getTraitSet().replace(BeamLogicalConvention.INSTANCE)),
calc.getProgram());
}
+
+ /**
+ * Returns true only if the literal can be correctly implemented by {@link
+ * org.apache.beam.sdk.extensions.sql.impl.rel.BeamCalcRel} in ZetaSQL.
+ */
+ private static boolean udfSupportsLiteral(RexLiteral rexLiteral) {
+ switch (rexLiteral.getType().getSqlTypeName()) {
+ case BIGINT:
+ case BOOLEAN:
+ case DECIMAL:
+ case DOUBLE:
+ case TIME:
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case VARBINARY:
Review comment:
Done. (Also changed literals for symmetry.)
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamJavaUdfCalcRule.java
##########
@@ -52,4 +117,57 @@ public RelNode convert(RelNode rel) {
RelOptRule.convert(input, input.getTraitSet().replace(BeamLogicalConvention.INSTANCE)),
calc.getProgram());
}
+
+ /**
+ * Returns true only if the literal can be correctly implemented by {@link
+ * org.apache.beam.sdk.extensions.sql.impl.rel.BeamCalcRel} in ZetaSQL.
+ */
+ private static boolean udfSupportsLiteral(RexLiteral rexLiteral) {
+ switch (rexLiteral.getType().getSqlTypeName()) {
+ case BIGINT:
+ case BOOLEAN:
+ case DECIMAL:
+ case DOUBLE:
+ case TIME:
+ case TIMESTAMP:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case VARBINARY:
+ case VARCHAR:
+ return true;
+ case DATE: // BEAM-11990
+ default:
+ return false;
+ }
+ }
+
+ /**
+ * Returns true only if the input ref can be correctly implemented by {@link
+ * org.apache.beam.sdk.extensions.sql.impl.rel.BeamCalcRel} in ZetaSQL.
+ */
+ private static boolean udfSupportsInput(RexInputRef rexInputRef) {
Review comment:
Done. (Also changed literals for symmetry.)
--
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] apilloud commented on a change in pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
apilloud commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r594728310
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,29 +174,20 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
- case BOOLEAN:
- case CHAR:
- case BINARY:
- case DECIMAL:
- break;
- default:
- // Reject unsupported literals
+ case BIGINT: // BEAM-11989
+ case DATE: // BEAM-11990
return false;
+ default:
+ break;
}
} else if (rexNode instanceof RexInputRef) {
SqlTypeName typeName = ((RexInputRef) rexNode).getType().getSqlTypeName();
switch (typeName) {
- case BIGINT:
- case DOUBLE:
- case BOOLEAN:
- case VARCHAR:
- case VARBINARY:
- case DECIMAL:
- break;
- default:
- // Reject unsupported input ref
+ case BIGINT: // BEAM-11989
Review comment:
BigInt was actually in the supported types for InputRef before...
----------------------------------------------------------------
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 #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r604436080
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
+ case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
Review comment:
done
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
+ case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
+ case ROW:
break;
+ case BIGINT: // BEAM-11989
+ case DATE: // BEAM-11990
default:
- // Reject unsupported literals
return false;
}
} else if (rexNode instanceof RexInputRef) {
SqlTypeName typeName = ((RexInputRef) rexNode).getType().getSqlTypeName();
switch (typeName) {
- case BIGINT:
case DOUBLE:
case BOOLEAN:
case VARCHAR:
case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
Review comment:
done
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
+ case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
+ case ROW:
break;
+ case BIGINT: // BEAM-11989
+ case DATE: // BEAM-11990
default:
- // Reject unsupported literals
return false;
}
} else if (rexNode instanceof RexInputRef) {
SqlTypeName typeName = ((RexInputRef) rexNode).getType().getSqlTypeName();
switch (typeName) {
- case BIGINT:
case DOUBLE:
case BOOLEAN:
case VARCHAR:
case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
+ case ROW:
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] apilloud commented on pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
apilloud commented on pull request #14241:
URL: https://github.com/apache/beam/pull/14241#issuecomment-799798976
With this approach, new types are accepted by default. What is the advantage of rejecting unsupported types rather than allowing supported types?
----------------------------------------------------------------
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] apilloud commented on a change in pull request #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
apilloud commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r594737271
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
Review comment:
This isn't a literal... CHAR is. Same with VARBINARY. https://github.com/apache/calcite/blob/039fe493e195416ee40c93b720f304ac9fc3c8c8/core/src/main/java/org/apache/calcite/rex/RexLiteral.java#L164
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
+ case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
Review comment:
I don't think there are ARRAY or ROW literals. I'm pretty sure both become calls to constructors.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,29 +174,20 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
- case BOOLEAN:
- case CHAR:
- case BINARY:
- case DECIMAL:
- break;
- default:
- // Reject unsupported literals
+ case BIGINT: // BEAM-11989
+ case DATE: // BEAM-11990
return false;
+ default:
+ break;
}
} else if (rexNode instanceof RexInputRef) {
SqlTypeName typeName = ((RexInputRef) rexNode).getType().getSqlTypeName();
switch (typeName) {
- case BIGINT:
- case DOUBLE:
- case BOOLEAN:
- case VARCHAR:
- case VARBINARY:
- case DECIMAL:
- break;
- default:
- // Reject unsupported input ref
+ case BIGINT: // BEAM-11989
Review comment:
Literals and input refs have a completely different implementation. BigInt is represented as a `BigInteger` in InputRef and a `BigDecimal` in Literals. (That is possibly a bug in Calcite.)
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
+ case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
+ case ROW:
break;
+ case BIGINT: // BEAM-11989
+ case DATE: // BEAM-11990
default:
- // Reject unsupported literals
return false;
}
} else if (rexNode instanceof RexInputRef) {
SqlTypeName typeName = ((RexInputRef) rexNode).getType().getSqlTypeName();
switch (typeName) {
- case BIGINT:
case DOUBLE:
case BOOLEAN:
case VARCHAR:
case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
Review comment:
I believe you need to recurse into `((RexInputRef) rexNode).getType().getComponentType()` to verify the array type is supported.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,28 +174,39 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
+ case DOUBLE:
case BOOLEAN:
- case CHAR:
- case BINARY:
+ case VARCHAR:
+ case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
+ case ROW:
break;
+ case BIGINT: // BEAM-11989
+ case DATE: // BEAM-11990
default:
- // Reject unsupported literals
return false;
}
} else if (rexNode instanceof RexInputRef) {
SqlTypeName typeName = ((RexInputRef) rexNode).getType().getSqlTypeName();
switch (typeName) {
- case BIGINT:
case DOUBLE:
case BOOLEAN:
case VARCHAR:
case VARBINARY:
case DECIMAL:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
+ case ROW:
Review comment:
I believe you need to recurse into `((RexInputRef) rexNode).getType().getFieldList()` to verify the struct only contains supported types.
----------------------------------------------------------------
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 #14241: [BEAM-11747] Narrow list of unsupported types in BeamJavaUdfCalcRule.
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #14241:
URL: https://github.com/apache/beam/pull/14241#discussion_r594734087
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSQLQueryPlanner.java
##########
@@ -174,29 +174,20 @@ static boolean hasOnlyJavaUdfInProjects(RelOptRuleCall x) {
} else if (rexNode instanceof RexLiteral) {
SqlTypeName typeName = ((RexLiteral) rexNode).getTypeName();
switch (typeName) {
- case NULL:
- case BOOLEAN:
- case CHAR:
- case BINARY:
- case DECIMAL:
- break;
- default:
- // Reject unsupported literals
+ case BIGINT: // BEAM-11989
+ case DATE: // BEAM-11990
return false;
+ default:
+ break;
}
} else if (rexNode instanceof RexInputRef) {
SqlTypeName typeName = ((RexInputRef) rexNode).getType().getSqlTypeName();
switch (typeName) {
- case BIGINT:
- case DOUBLE:
- case BOOLEAN:
- case VARCHAR:
- case VARBINARY:
- case DECIMAL:
- break;
- default:
- // Reject unsupported input ref
+ case BIGINT: // BEAM-11989
Review comment:
I noticed that, but I wasn't sure why it is safe to include in input refs but not literals?
----------------------------------------------------------------
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