[BEAM-10438] Update SupportedZetaSqlBuiltinFunctions and support math functions #12643
Conversation
|
There are tests failing due to this change. |
|
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! |
|
OK now the tests all pass and the blocking bug is fixed. |
|
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. |
|
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. |
| 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"; |
There was a problem hiding this 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.)
There was a problem hiding this comment.
I agree. I will let this change get in and avoid similar changes in the future. Sorry for the inconvenience.
| // FunctionSignatureId.FN_ABS_INT64, // abs | ||
| // FunctionSignatureId.FN_ABS_DOUBLE, // abs | ||
| // FunctionSignatureId.FN_ABS_NUMERIC, // abs | ||
| FunctionSignatureId.FN_ABS_INT64, // abs |
There was a problem hiding this comment.
nit: This PR has a interesting mix of minor refactors and addition of new functionality. These should probably be separate changes.
There was a problem hiding this comment.
Make sense. I will remember to separate different categories of changes in the future. Thanks for the suggestion!
| @@ -475,17 +512,22 @@ | |||
|
|
|||
| // FunctionSignatureId.FN_RANGE_BUCKET, // range_bucket(T, array<T>) -> int64 | |||
|
|
|||
| // FunctionSignatureId.FN_RAND, // rand() -> double | |||
| FunctionSignatureId.FN_RAND // rand() -> double | |||
There was a problem hiding this 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/
There was a problem hiding this comment.
Thanks for catching this! I will disable this for now, and enable it later once we have a thorough test.
| @@ -24,6 +24,11 @@ | |||
| @Internal | |||
| public class ZetaSqlTypesUtils { | |||
|
|
|||
| public static final BigDecimal NUMERIC_MAX_VALUE = | |||
There was a problem hiding this comment.
nit: these constants are only used in ZetaSqlMathFunctionsTest.java, do they belong there?
r: @apilloud
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.