Fix return types of MOD and DIVIDE UDFs#3513
Conversation
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
Signed-off-by: Yuanchun Shen <yuanchu@amazon.com>
dai-chen
left a comment
There was a problem hiding this comment.
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.
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.
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.
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!
I updated the description to explain what else I modified. Thanks for reminding me! |
Swiddis
left a comment
There was a problem hiding this comment.
Some thoughts, one suggestion I'd like to see either revised or documented
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.