[BEAM-8057] Support ZetaSQL DOUBLE +INF, -INF and NAN by ZijieSong946 · Pull Request #12292 · apache/beam

ZijieSong946 · 2020-07-17T03:23:08Z

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.

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

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

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

Post-Commit Tests Status (on master branch)

Lang	Dataflow	Samza	Twister2
Go	---	---	---
Java
Python		---	---
XLang	---	---	---

Pre-Commit Tests Status (on master branch)

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

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

robinyqiu

It's great to see this long-standing issue fixed. Thank you for working on this, Zijie. Please also link to your doc from here after you finish that. Left some comments to further simplify the code.

apilloud

LGTM.

I love this approach! It is very clean and keeps most doubles as normal Calcite literals.

ZijieSong946 · 2020-07-17T18:59:47Z

LGTM.

I love this approach! It is very clean and keeps most doubles as normal Calcite literals.

Great. Thanks a lot.

robinyqiu · 2020-07-17T19:40:03Z

I am testing this internally, after the tests pass I will merge it.

ZijieSong946 · 2020-07-17T19:40:50Z

I am testing this internally, after the tests pass I will merge it.

Thanks.

robinyqiu · 2020-07-17T22:12:26Z

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.

apilloud · 2020-07-17T22:18:21Z

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

apilloud · 2020-07-17T22:19:42Z

(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.)

robinyqiu · 2020-07-17T22:35:03Z

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)

robinyqiu · 2020-07-17T22:40:07Z

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?

ZijieSong946 · 2020-07-18T04:06:22Z

(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.

ZijieSong946 · 2020-07-18T04:07:23Z

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.

ZijieSong946 · 2020-07-21T20:26:34Z

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. (apache/calcite#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

robinyqiu · 2020-07-22T05:41:18Z

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.

ZijieSong946 · 2020-07-22T21:01:54Z

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.

robinyqiu · 2020-07-22T21:47:14Z

Thank you! I will merge it now (tested internally).

ZijieSong946 · 2020-08-17T22:57:59Z

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

probot-autolabeler Bot added extensions java sql labels Jul 17, 2020

robinyqiu approved these changes Jul 17, 2020

View reviewed changes

robinyqiu reviewed Jul 17, 2020

View reviewed changes

Comment thread ...n/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java Outdated

ZijieSong946 force-pushed the DoubleBugFixed branch from 7e8f3ac to c1b4f88 Compare July 17, 2020 18:49

apilloud approved these changes Jul 17, 2020

View reviewed changes

robinyqiu reviewed Jul 22, 2020

View reviewed changes

Comment thread ...rc/main/java/org/apache/beam/sdk/extensions/sql/zetasql/translation/ExpressionConverter.java Outdated

Support ZetaSQL DOUBLE +inf, -inf and NaN.

0bff5fc

ZijieSong946 force-pushed the DoubleBugFixed branch from 585939d to 0bff5fc Compare July 22, 2020 21:00

robinyqiu merged commit 1e569fa into apache:master Jul 22, 2020

Uh oh!

Conversation

ZijieSong946 commented Jul 17, 2020

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

Uh oh!

robinyqiu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apilloud left a comment

Choose a reason for hiding this comment

Uh oh!

ZijieSong946 commented Jul 17, 2020

Uh oh!

robinyqiu commented Jul 17, 2020

Uh oh!

ZijieSong946 commented Jul 17, 2020

Uh oh!

robinyqiu commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apilloud commented Jul 17, 2020

Uh oh!

apilloud commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robinyqiu commented Jul 17, 2020

Uh oh!

robinyqiu commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZijieSong946 commented Jul 18, 2020

Uh oh!

ZijieSong946 commented Jul 18, 2020

Uh oh!

ZijieSong946 commented Jul 21, 2020

Uh oh!

robinyqiu commented Jul 22, 2020

Uh oh!

Uh oh!

ZijieSong946 commented Jul 22, 2020

Uh oh!

robinyqiu commented Jul 22, 2020

Uh oh!

ZijieSong946 commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

robinyqiu commented Jul 17, 2020 •

edited

Loading

apilloud commented Jul 17, 2020 •

edited

Loading

robinyqiu commented Jul 17, 2020 •

edited

Loading

ZijieSong946 commented Aug 17, 2020 •

edited

Loading