-
Couldn't load subscription status.
- Fork 176
Fix return types of MOD and DIVIDE UDFs #3513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think not all changes are not covered in the PR description?
| 3.142857142857143, | ||
| 3.0, | ||
| 2.4333334, | ||
| 0.8225806704669051)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you confirm this won't be flaky due to floating precision? I see error = 1e-10 in `closeTo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's likely. I think I'd better replace it with a calculated value, e.g. 22.0 / 7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that the returned representation from calcite is already truncated. See OpenSearchExecutionEngine.java#L147 and JdbcOpenSearchDataTypeConvertor.java#L67.
In the line 67
Object value = rs.getObject(i);
The value for e.g. 7.3 / 6.2 is returned as Float(1.1774194). Therefore, it will fail tests if the expected value is set to the precise computed results (around 1.1774193548387095 in this case).
That was why I used the literal values which exactly match its actual result in tests.
This is also how previous versions before introducing calcite behaves (opensearch-sql 2.19.1.0).
IMO, it might not to the user's interests whether they are shown a more precise value with an error smaller to 1e-10, or a truncated value with an error around 1e-5.
Admittedly, the current implementation is not resilient to changes in underlying implementations. But I can not come up with a better solution than using literal values in tests. Please feel free to leave your thoughts!
core/src/main/java/org/opensearch/sql/calcite/utils/MathUtils.java
Outdated
Show resolved
Hide resolved
I updated the description to explain what else I modified. Thanks for reminding me! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts, one suggestion I'd like to see either revised or documented
core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/ModFunction.java
Show resolved
Hide resolved
core/src/main/java/org/opensearch/sql/calcite/udf/mathUDF/DivideFunction.java
Outdated
Show resolved
Hide resolved
integ-test/src/test/java/org/opensearch/sql/calcite/standalone/CalcitePPLBuiltinFunctionIT.java
Show resolved
Hide resolved
Could you check this? @yuancu |
- additionally update expressions.rst to clarify how DIVIDE handles division by zero Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
* Fix divide function with a UDF Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Coerce return types for DIVIDE and MOD UDFs Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> * Remove the magic threshold for DIVIDE and MOD - additionally update expressions.rst to clarify how DIVIDE handles division by zero Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> --------- Signed-off-by: Yuanchun Shen <yuanchu@amazon.com> Signed-off-by: xinyual <xinyual@amazon.com>
Problems
Division by 0 will raise the following exception:
Mod does not return wider types for
BYTE&SHORT. I.e.MOD(BYTE, SHORT) -> INTEGER, but is expected to returnSHORT(a wider type between operands, as what is returned in opensearch-sql v2.x)Fix
Fix
/function with a UDF. It returnsnullif the divisor is 0.The rest behaviors is aligned with original implementation with
SqlStdOperatorTable.DIVIDE& opensearch-sql v2.x in terms of return types and integral division.I.e.
(BYTE, SHORT) -> SHORT(INTEGER, LONG) -> LONGno matter it is dividable or not.Fixed the return types of
MODfunction such that it returns a value with the least restrictive type between its operands. E.g.(SHORT, BYTE) -> SHORTAdditionally, this PR did a refactor for TimeAddSub UDF -- I moved a util function from UDFUtils to
TimeAddSubFunction.java, as the util function is only used by TimeAdd & TimeSub. This also aligns with DateAddSubFunction -- it has a similar util function placed within the UDF definition.Related Issues
Relevant to #3390
Check List
--signoff.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.