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/08/20 00:05:52 UTC
[GitHub] [beam] robinyqiu opened a new pull request #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
robinyqiu opened a new pull request #12643:
URL: https://github.com/apache/beam/pull/12643
1. Sync with builtin_function.proto in ZetaSQL 2020.08.1 release
2. Turn on all scalar math functions supported by ZetaSQL evaluator and add unit tests for them
3. Move all existing unit tests for math functions to ZetaSqlMathFunctionsTest
4. Some minor style change
r: @apilloud
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/)<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/)
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)
------------------------------------------------------------------------------------------------
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] robinyqiu commented on pull request #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12643:
URL: https://github.com/apache/beam/pull/12643#issuecomment-681186675
That's totally fine for me. Please take your time for the review.
I don't think the first commit is urgent. Let's wait until you find time to review both. Also, I have other works to do so I am not blocked on this.
----------------------------------------------------------------
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 #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
Posted by GitBox <gi...@apache.org>.
apilloud commented on a change in pull request #12643:
URL: https://github.com/apache/beam/pull/12643#discussion_r489084327
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -87,16 +87,17 @@
.put("$extract_time", "TIME")
.put("$extract_datetime", "DATETIME")
.build();
- public static final String DOUBLE_POSITIVE_INF_FUNCTION = "double_positive_inf";
- public static final String DOUBLE_NEGATIVE_INF_FUNCTION = "double_negative_inf";
- public static final String DOUBLE_NAN_FUNCTION = "double_nan";
- private static final Map<String, String> DOUBLE_FUNCTIONS =
+ public static final String DOUBLE_POSITIVE_INF_WRAPPER = "double_positive_inf";
Review comment:
nit: Renames like this are probably a net negative when you take into account the total cost of a change (time authoring and reviewing, increased diffs when debugging future issues, potential upgrade friction for customers). It is hard to know exactly where the line is so feel free to push forward with these as much of the work is already done. In the future I would suggest avoiding non-functional changes, particularly if they are something that would be at most a nit on a code review. (These went in recently and didn't even get a nit comment.)
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SupportedZetaSqlBuiltinFunctions.java
##########
@@ -475,17 +512,22 @@
// FunctionSignatureId.FN_RANGE_BUCKET, // range_bucket(T, array<T>) -> int64
- // FunctionSignatureId.FN_RAND, // rand() -> double
+ FunctionSignatureId.FN_RAND // rand() -> double
Review comment:
This one scares me just a little. In particular, I'm worried that we might accidentally optimize such that we end up with a constant. Is that something we need to test? https://xkcd.com/221/
##########
File path: sdks/java/extensions/sql/zetasql/src/test/java/org/apache/beam/sdk/extensions/sql/zetasql/ZetaSqlTypesUtils.java
##########
@@ -24,6 +24,11 @@
@Internal
public class ZetaSqlTypesUtils {
+ public static final BigDecimal NUMERIC_MAX_VALUE =
Review comment:
nit: these constants are only used in `ZetaSqlMathFunctionsTest.java`, do they belong there?
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SupportedZetaSqlBuiltinFunctions.java
##########
@@ -277,29 +293,31 @@
FunctionSignatureId.FN_PARSE_DATETIME, // parse_datetime
FunctionSignatureId.FN_PARSE_TIME, // parse_time
FunctionSignatureId.FN_PARSE_TIMESTAMP, // parse_timestamp
+ // FunctionSignatureId.FN_LAST_DAY_DATE, // last_day date
+ // FunctionSignatureId.FN_LAST_DAY_DATETIME, // last_day datetime
// Math functions
- // FunctionSignatureId.FN_ABS_INT64, // abs
- // FunctionSignatureId.FN_ABS_DOUBLE, // abs
- // FunctionSignatureId.FN_ABS_NUMERIC, // abs
+ FunctionSignatureId.FN_ABS_INT64, // abs
Review comment:
nit: This PR has a interesting mix of minor refactors and addition of new functionality. These should probably be separate changes.
----------------------------------------------------------------
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 #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12643:
URL: https://github.com/apache/beam/pull/12643#issuecomment-677952161
Yes I realized that. The one on Jenkins is due to double comparison. There is also compliance failures that got revealed from this change. I am working on the fix now. You can hold on the review. 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 #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on pull request #12643:
URL: https://github.com/apache/beam/pull/12643#issuecomment-680204320
OK now the tests all pass and the blocking [bug](https://issues.apache.org/jira/browse/BEAM-10783) is fixed.
----------------------------------------------------------------
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 #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
Posted by GitBox <gi...@apache.org>.
apilloud commented on pull request #12643:
URL: https://github.com/apache/beam/pull/12643#issuecomment-681178294
Sorry I haven't gotten to this yet. My schedule is busy (performance reviews and on call rotations) through September 16th, so it might be a little while before I have time to fit in a large review. I can LGTM the first commit immediately. If there is anything else you can break into a smaller piece that might help.
----------------------------------------------------------------
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 #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
Posted by GitBox <gi...@apache.org>.
apilloud commented on pull request #12643:
URL: https://github.com/apache/beam/pull/12643#issuecomment-677947904
There are tests failing due to this change.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] robinyqiu commented on a change in pull request #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
Posted by GitBox <gi...@apache.org>.
robinyqiu commented on a change in pull request #12643:
URL: https://github.com/apache/beam/pull/12643#discussion_r512460592
##########
File path: sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/meta/provider/bigquery/BeamBigQuerySqlDialect.java
##########
@@ -87,16 +87,17 @@
.put("$extract_time", "TIME")
.put("$extract_datetime", "DATETIME")
.build();
- public static final String DOUBLE_POSITIVE_INF_FUNCTION = "double_positive_inf";
- public static final String DOUBLE_NEGATIVE_INF_FUNCTION = "double_negative_inf";
- public static final String DOUBLE_NAN_FUNCTION = "double_nan";
- private static final Map<String, String> DOUBLE_FUNCTIONS =
+ public static final String DOUBLE_POSITIVE_INF_WRAPPER = "double_positive_inf";
Review comment:
I agree. I will let this change get in and avoid similar changes in the future. Sorry for the inconvenience.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SupportedZetaSqlBuiltinFunctions.java
##########
@@ -475,17 +512,22 @@
// FunctionSignatureId.FN_RANGE_BUCKET, // range_bucket(T, array<T>) -> int64
- // FunctionSignatureId.FN_RAND, // rand() -> double
+ FunctionSignatureId.FN_RAND // rand() -> double
Review comment:
Thanks for catching this! I will disable this for now, and enable it later once we have a thorough test.
##########
File path: sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/SupportedZetaSqlBuiltinFunctions.java
##########
@@ -277,29 +293,31 @@
FunctionSignatureId.FN_PARSE_DATETIME, // parse_datetime
FunctionSignatureId.FN_PARSE_TIME, // parse_time
FunctionSignatureId.FN_PARSE_TIMESTAMP, // parse_timestamp
+ // FunctionSignatureId.FN_LAST_DAY_DATE, // last_day date
+ // FunctionSignatureId.FN_LAST_DAY_DATETIME, // last_day datetime
// Math functions
- // FunctionSignatureId.FN_ABS_INT64, // abs
- // FunctionSignatureId.FN_ABS_DOUBLE, // abs
- // FunctionSignatureId.FN_ABS_NUMERIC, // abs
+ FunctionSignatureId.FN_ABS_INT64, // abs
Review comment:
Make sense. I will remember to separate different categories of changes in the future. Thanks for the suggestion!
----------------------------------------------------------------
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 #12643: [BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions
Posted by GitBox <gi...@apache.org>.
robinyqiu merged pull request #12643:
URL: https://github.com/apache/beam/pull/12643
----------------------------------------------------------------
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