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/17 03:23:18 UTC
[GitHub] [beam] ZijieSong946 opened a new pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
ZijieSong946 opened a new pull request #12292:
URL: https://github.com/apache/beam/pull/12292
**This PR supports `+inf`, `-inf` and `NaN` for ZetaSQL DOUBLE type.**
- Original ignored test case `ZetaSqlDialectSpecTest.testEQ2` passed.
- `IS_INF()` and `IS_NAN()` are supported for ZetaSQL now.
- Additional corresponding test cases added.
r: @apilloud @robinyqiu
------------------------
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/)<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_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_Direct/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
--- | --- | --- | --- | ---
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] ZijieSong946 commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r459080788
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -805,6 +805,9 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
if (wrapperFun == null) {
ret = rexBuilder().makeApproxLiteral(new BigDecimal(val), returnType);
} else if (BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION.equals(wrapperFun)) {
+ // 'https://issues.apache.org/jira/browse/BEAM-10550'
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] robinyqiu commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-662714298
Thank you! I will merge it now (tested internally).
----------------------------------------------------------------
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 #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
apilloud commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660361089
(There are a few other config parameters in that file, one should stop the optimizer from affecting `NaN` if isDeterministic doesn't.)
----------------------------------------------------------------
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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456572768
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isInfinite(val) && val < 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isNaN(val)) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else {
+ ret =
+ rexBuilder()
+ .makeApproxLiteral(
+ new BigDecimal(val),
+ ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
Review comment:
I meant
```
String wrapperFun = null;
if (val == Double.POSITIVE_INFINITY) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION;
} else if (val == Double.NEGATIVE_INFINITY) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION;
} else if (Double.isNan(val)) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION;
}
RelDataType returnType = ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder());
if (wrapperFun == null) {
ret = rexBuilder().makeApproxLiteral(new BigDecimal(val), returnType);
} else {
ret = rexBuilder().makeCall(SqlOperators.createOtherKindSqlFunction(wrapperFun, returnType.getSqlTypeName()));
}
```
Isn't this simpler :)
----------------------------------------------------------------
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 #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
apilloud commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660360679
Instead of adding an argument with a random number, make isDeterministic() false: https://github.com/apache/calcite/blob/551e3f5a182a48e20586e40c378224bb63e4bfb3/core/src/main/java/org/apache/calcite/sql/SqlOperator.java#L940
----------------------------------------------------------------
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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456572768
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isInfinite(val) && val < 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isNaN(val)) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else {
+ ret =
+ rexBuilder()
+ .makeApproxLiteral(
+ new BigDecimal(val),
+ ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
Review comment:
I meant
```
String wrapperFun = null;
if (val == Double.POSITIVE_INFINITY) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION;
} else if (val == Double.NEGATIVE_INFINITY) {
BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION
} else if (Double.isNan(val)) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION;
}
RelDataType returnType = ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder());
if (wrapperFun == null) {
ret = rexBuilder().makeApproxLiteral(new BigDecimal(val), returnType);
} else {
ret = rexBuilder().makeCall(SqlOperators.createOtherKindSqlFunction(wrapperFun, returnType.getSqlTypeName()));
}
```
Isn't this simpler :)
----------------------------------------------------------------
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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r458552857
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -805,6 +805,9 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
if (wrapperFun == null) {
ret = rexBuilder().makeApproxLiteral(new BigDecimal(val), returnType);
} else if (BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION.equals(wrapperFun)) {
+ // 'https://issues.apache.org/jira/browse/BEAM-10550'
Review comment:
Usually I will make a TODO here (such that you can easily find it in IntelliJ), like:
```
// TODO[BEAM-10550] Update the temporary workaround below after vendored Calcite version >= 1.23.0
```
----------------------------------------------------------------
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] robinyqiu commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-662253141
Thank you Zijie for the investigation. The temporary solution LGTM. I assigned the JIRA to myself because as discussed in meeting I will be updating the vendored Calcite version.
This PR failed building because I made some changes to some functions your code depends on. You will need to rebase against master and fix 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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r458552857
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -805,6 +805,9 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
if (wrapperFun == null) {
ret = rexBuilder().makeApproxLiteral(new BigDecimal(val), returnType);
} else if (BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION.equals(wrapperFun)) {
+ // 'https://issues.apache.org/jira/browse/BEAM-10550'
Review comment:
Usually I will make a TODO here (such that you can easily find it in the IntelliJ TODO tab), like:
```
// TODO[BEAM-10550]: Update the temporary workaround below after vendored Calcite version >= 1.23.0
```
----------------------------------------------------------------
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] ZijieSong946 commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456593995
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isInfinite(val) && val < 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isNaN(val)) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else {
+ ret =
+ rexBuilder()
+ .makeApproxLiteral(
+ new BigDecimal(val),
+ ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
Review comment:
That's seems good idea. Got 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] robinyqiu commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660303025
I am testing this internally, after the tests pass I will merge 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] ZijieSong946 commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660420822
> Oh the last comment was replying to your first comment. If other config parameters can do the work we should definitely do that. @ZijieSong946 Could you investigate this a bit next week?
Got it. I would figure out a possible solution to handle that issue.
----------------------------------------------------------------
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] ZijieSong946 commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-662088537
I dug into this problem and figured out that we should avoid hitting into the operation simplification branch here (https://github.com/apache/calcite/blob/3530daaa8cad43aad6845b6c79e4bc1ca0e72f5f/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L325).
For the current Calcite version we use (1.20), there is no check for `isDeterministic()`, it is added on Mar.6. (https://github.com/apache/calcite/pull/1818)
So, we have to make `equals()` to be false. As for `RexLiteral.equals()`, it checks if the type and the value equals. (https://github.com/apache/calcite/blob/3530daaa8cad43aad6845b6c79e4bc1ca0e72f5f/core/src/main/java/org/apache/calcite/rex/RexLiteral.java#L1084-L1088)
I decided to add a random parameter to the `Nan` wrapper function for make two `Nan` different in value to avoid operation simplification in Calcite.
@robinyqiu @apilloud
----------------------------------------------------------------
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] ZijieSong946 commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-662694483
> Thank you Zijie for the investigation. The temporary solution LGTM. I assigned the JIRA to myself because as discussed in meeting I will be updating the vendored Calcite version.
>
> This PR failed building because I made some changes to some functions your code depends on. You will need to rebase against master and fix it.
Rebased.
----------------------------------------------------------------
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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456207986
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isInfinite(val) && val < 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isNaN(val)) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else {
+ ret =
+ rexBuilder()
+ .makeApproxLiteral(
+ new BigDecimal(val),
+ ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
Review comment:
I could create 2 local parameters: `String wrapperFun` (null for the last case) and `RelDataType returnType` to simplify this big if-else block.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
Review comment:
How about just check `val == Double.POSITIVE_INFINITY` here? (similar below)
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -157,8 +160,20 @@ public void unparseCall(
break;
case OTHER_FUNCTION:
String funName = call.getOperator().getName();
- if (NUMERIC_LITERAL_FUNCTION.equals(funName)) {
- // self-designed function dealing with the unparsing of ZetaSQL numeric literal
+ if (DOUBLE_POSITIVE_INF_FUNCTION.equals(funName)) {
Review comment:
If you create a map `DOUBLE_LITERAL_FUNCTIONS`, you can merge the 3 branches here and the 3 functions below. (Similar to what I did with `EXTRACT_FUNCTIONS`)
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -239,6 +254,18 @@ private void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec, int right
writer.endFunCall(trimFrame);
}
+ private void unparseDoublePositiveINFWrapperFunction(SqlWriter writer) {
+ writer.literal("CAST('+inf' AS FLOAT64)");
Review comment:
Maybe comment here: "There is no direct ZetaSQL literal representation of NaN or infinity"?
----------------------------------------------------------------
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 edited a comment on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
apilloud edited a comment on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660361089
(There are a few other config parameters in that file, one should stop the optimizer from affecting `NaN` if isDeterministic doesn't. Looks like `isDynamicFunction` is used by RAND and CURRENT_TIME.)
----------------------------------------------------------------
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] ZijieSong946 commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660303337
> I am testing this internally, after the tests pass I will merge it.
Thanks.
----------------------------------------------------------------
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] robinyqiu commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660358873
Actually this change unveils a data corruption bug (produces wrong result).
It is related to NaN comparison (=, !=, >, >=, <, and <=). For example, if you have a query like
```
SELECT CAST('NAN' AS FLOAT64) = CAST('NAN' AS FLOAT64)
```
the expected result is `false` because according to ZetaSQL definition, `NaN` compared to **any** value should return false, but our engine returns true (please add a test to reproduce this.)
I dug into this a bit and found it is because Calcite internally does simplification to comparison operations (https://github.com/apache/calcite/blob/3fb68f6c22a7bcbc4cb1fff114bc911b1e31c4de/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L336-L351). So the above expression will be simplified to (you can print the sql query in `BeamZetaSqlCalcRel` to verify yourself)
```
NULL OR CAST('NAN' AS FLOAT64) IS NOT NULL
```
The simplification is triggered because the 2 operands of `=` are "equal" (they are both a `RexCall` to a function named `double_nan` with no input).
One way I can think of to fix this, without affecting other code path, is that we can generate a random double and convert it to `BigDecimal` and make it a input to `double_nan`. This input parameter is never used, its sole purpose is to make Calcite treat two calls to `double_nan` different.
----------------------------------------------------------------
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] ZijieSong946 commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456603812
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -157,8 +166,13 @@ public void unparseCall(
break;
case OTHER_FUNCTION:
String funName = call.getOperator().getName();
- if (NUMERIC_LITERAL_FUNCTION.equals(funName)) {
- // self-designed function dealing with the unparsing of ZetaSQL numeric literal
+ if (DOUBLE_FUNCTIONS.containsKey(funName)) {
+ // self-designed function dealing with the unparsing of ZetaSQL DOUBLE positive
+ // infinity, negative infinity and NaN
+ unparseDoubleWrapperFunction(writer, call);
Review comment:
Acknowledged.
----------------------------------------------------------------
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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456602411
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -157,8 +166,13 @@ public void unparseCall(
break;
case OTHER_FUNCTION:
String funName = call.getOperator().getName();
- if (NUMERIC_LITERAL_FUNCTION.equals(funName)) {
- // self-designed function dealing with the unparsing of ZetaSQL numeric literal
+ if (DOUBLE_FUNCTIONS.containsKey(funName)) {
+ // self-designed function dealing with the unparsing of ZetaSQL DOUBLE positive
+ // infinity, negative infinity and NaN
+ unparseDoubleWrapperFunction(writer, call);
Review comment:
Actually you can just pass `funName` to 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] ZijieSong946 commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456599686
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -157,8 +160,20 @@ public void unparseCall(
break;
case OTHER_FUNCTION:
String funName = call.getOperator().getName();
- if (NUMERIC_LITERAL_FUNCTION.equals(funName)) {
- // self-designed function dealing with the unparsing of ZetaSQL numeric literal
+ if (DOUBLE_POSITIVE_INF_FUNCTION.equals(funName)) {
Review comment:
Done. Thanks for pointing it out.
----------------------------------------------------------------
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] ZijieSong946 commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-675155529
The detailed designs for overcoming the inconsistency between Calcite and ZetaSQL DOUBLE +inf/-inf/NaN literal representation:
https://docs.google.com/document/d/1veKaDTMttU2q2Izss--Y5Cagg2JUSEA1dS3NVxYjnYg/edit#
----------------------------------------------------------------
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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456603080
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -157,8 +166,13 @@ public void unparseCall(
break;
case OTHER_FUNCTION:
String funName = call.getOperator().getName();
- if (NUMERIC_LITERAL_FUNCTION.equals(funName)) {
- // self-designed function dealing with the unparsing of ZetaSQL numeric literal
+ if (DOUBLE_FUNCTIONS.containsKey(funName)) {
+ // self-designed function dealing with the unparsing of ZetaSQL DOUBLE positive
+ // infinity, negative infinity and NaN
+ unparseDoubleWrapperFunction(writer, call);
Review comment:
And after you change this, could you please do a rebase again?
----------------------------------------------------------------
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] ZijieSong946 commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660420719
> (There are a few other config parameters in that file, one should stop the optimizer from affecting `NaN` if isDeterministic doesn't. Looks like `isDynamicFunction` is used by RAND and CURRENT_TIME.)
Acknowledged.
----------------------------------------------------------------
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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456572768
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isInfinite(val) && val < 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isNaN(val)) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else {
+ ret =
+ rexBuilder()
+ .makeApproxLiteral(
+ new BigDecimal(val),
+ ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
Review comment:
I meant
```
String wrapperFun = null;
if (val == Double.POSITIVE_INFINITY) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION;
} else if (val == Double.NEGATIVE_INFINITY) {
BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION
} else if (Double.isNan(val)) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION;
}
RelDataType returnType = ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder());
if (wrapperFun == null) {
ret = rexBuilder().makeApproxLiteral(new BigDecimal(val), returnType);
} else {
ret = rexBuilder().makeCall(SqlOperators.createOtherKindSqlFunction(wrapperFun, returnType.getSqlTypeName()));
}
```
----------------------------------------------------------------
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] ZijieSong946 commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456599838
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -239,6 +254,18 @@ private void unparseTrim(SqlWriter writer, SqlCall call, int leftPrec, int right
writer.endFunCall(trimFrame);
}
+ private void unparseDoublePositiveINFWrapperFunction(SqlWriter writer) {
+ writer.literal("CAST('+inf' AS FLOAT64)");
Review comment:
Comment Added.
----------------------------------------------------------------
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] ZijieSong946 commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660285690
> LGTM.
>
> I love this approach! It is very clean and keeps most doubles as normal Calcite literals.
Great. Thanks a lot.
----------------------------------------------------------------
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] ZijieSong946 edited a comment on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 edited a comment on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-675155529
The detailed designs for overcoming the inconsistency between Calcite and ZetaSQL DOUBLE +inf/-inf/NaN literal representation:
https://docs.google.com/document/d/1veKaDTMttU2q2Izss--Y5Cagg2JUSEA1dS3NVxYjnYg/edit?usp=sharing
----------------------------------------------------------------
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] ZijieSong946 commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456599303
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
Review comment:
Thanks. Done.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isInfinite(val) && val < 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isNaN(val)) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else {
+ ret =
+ rexBuilder()
+ .makeApproxLiteral(
+ new BigDecimal(val),
+ ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
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] robinyqiu merged pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu merged pull request #12292:
URL: https://github.com/apache/beam/pull/12292
----------------------------------------------------------------
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] robinyqiu commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456572768
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isInfinite(val) && val < 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isNaN(val)) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else {
+ ret =
+ rexBuilder()
+ .makeApproxLiteral(
+ new BigDecimal(val),
+ ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
Review comment:
I meant
```
String wrapperFun = null;
if (val == Double.POSITIVE_INFINITY) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION;
} else if (val == Double.NEGATIVE_INFINITY) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION
} else if (Double.isNan(val)) {
wrapperFun = BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION;
}
RelDataType returnType = ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder());
if (wrapperFun == null) {
ret = rexBuilder().makeApproxLiteral(new BigDecimal(val), returnType);
} else {
ret = rexBuilder().makeCall(SqlOperators.createOtherKindSqlFunction(wrapperFun, returnType.getSqlTypeName()));
}
```
Isn't this simpler :)
----------------------------------------------------------------
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] robinyqiu edited a comment on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu edited a comment on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660358873
Actually this change unveils a data corruption bug (produces wrong result).
It is related to NaN comparison (=, !=, >, >=, <, and <=). For example, if you have a query like
```
SELECT CAST('NAN' AS FLOAT64) = CAST('NAN' AS FLOAT64)
```
the expected result is `false` because according to ZetaSQL definition, `NaN` compared to **any** value should return false, but our engine returns true (please add a test to reproduce this).
I dug into this a bit and found it is because Calcite internally does simplification to comparison operations (https://github.com/apache/calcite/blob/3fb68f6c22a7bcbc4cb1fff114bc911b1e31c4de/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L336-L351). So the above expression will be simplified to (you can print the sql query in `BeamZetaSqlCalcRel` to verify yourself)
```
NULL OR CAST('NAN' AS FLOAT64) IS NOT NULL
```
The simplification is triggered because the 2 operands of `=` are "equal" (they are both a `RexCall` to a function named `double_nan` with no input).
One way I can think of to fix this, without affecting other code path, is that we can generate a random double and convert it to `BigDecimal` and make it a input to `double_nan`. This input parameter is never used, its sole purpose is to make Calcite treat two calls to `double_nan` different.
----------------------------------------------------------------
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] ZijieSong946 commented on a change in pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
ZijieSong946 commented on a change in pull request #12292:
URL: https://github.com/apache/beam/pull/12292#discussion_r456545841
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java
##########
@@ -786,15 +786,39 @@ private RexNode convertSimpleValueToRexNode(TypeKind kind, Value value) {
ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
break;
case TYPE_DOUBLE:
+ // Cannot simply call makeApproxLiteral() for ZetaSQL DOUBLE type because positive infinity,
+ // negative infinity and Nan cannot be directly converted to BigDecimal. So we create three
+ // wrapper functions here for these three cases such that we can later recognize it and
+ // customize its unparsing in BeamBigQuerySqlDialect.
double val = value.getDoubleValue();
- if (Double.isInfinite(val) || Double.isNaN(val)) {
- throw new UnsupportedOperationException("Does not support Infinite or NaN literals.");
+ if (Double.isInfinite(val) && val > 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_POSITIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isInfinite(val) && val < 0) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NEGATIVE_INF_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else if (Double.isNaN(val)) {
+ ret =
+ rexBuilder()
+ .makeCall(
+ SqlOperators.createOtherKindSqlFunction(
+ BeamBigQuerySqlDialect.DOUBLE_NAN_FUNCTION,
+ ZetaSqlCalciteTranslationUtils.toCalciteTypeName(kind)));
+ } else {
+ ret =
+ rexBuilder()
+ .makeApproxLiteral(
+ new BigDecimal(val),
+ ZetaSqlCalciteTranslationUtils.toSimpleRelDataType(kind, rexBuilder()));
Review comment:
I am not quiet sure how to merge the first three blocks with the last one, because the first three use `resBuilder().makeCall()` while the last one uses `rexBuilder().makeApproxLiteral()`. Also, `returnType` seems can only be used in the last block.
----------------------------------------------------------------
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] robinyqiu edited a comment on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu edited a comment on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660367106
Oh the last comment was replying to your first comment. If other config parameters can do the work we should definitely do that. @ZijieSong946 Could you investigate this a bit next week?
----------------------------------------------------------------
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] robinyqiu commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660365698
Yeah that is definitely a better approach. However checking of `isDeterministic()` in `RexSimplify` (https://github.com/apache/calcite/blob/551e3f5a182a48e20586e40c378224bb63e4bfb3/core/src/main/java/org/apache/calcite/rex/RexSimplify.java#L334) is only enabled in Calcite 1.23.0 (See https://issues.apache.org/jira/browse/CALCITE-3809).
So how about we use the old approach for now, and create a JIRA for updating to the new approach after we update to 1.23? (The JIRA should be marked blocked by: https://issues.apache.org/jira/browse/BEAM-9379 (we should change 1.22 to 1.23)
----------------------------------------------------------------
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] robinyqiu commented on pull request #12292: [BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12292:
URL: https://github.com/apache/beam/pull/12292#issuecomment-660367106
Oh the last comment was replying to your first comment. If other config parameters can do the work we can definitely do that. @ZijieSong946 Could you investigate this a bit next week?
----------------------------------------------------------------
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