-
Notifications
You must be signed in to change notification settings - Fork 594
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
fix(udf): avoid panic on invalid output and fix decimal output scale lost #13828
Conversation
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
let data_chunk = | ||
DataChunk::try_from(&output).expect("failed to convert UDF output to DataChunk"); | ||
let data_chunk = DataChunk::try_from(&output)?; |
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.
What happens if we only change this line without changing udf sdk? 🤔
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.
will result in expression error: invalid decimal
, or silently produce wrong value 1e-10
-> 1.000...
Signed-off-by: Runji Wang <[email protected]>
Signed-off-by: Runji Wang <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #13828 +/- ##
==========================================
- Coverage 68.24% 68.23% -0.01%
==========================================
Files 1525 1525
Lines 262214 262212 -2
==========================================
- Hits 178946 178920 -26
- Misses 83268 83292 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM
…lost (#13828) Signed-off-by: Runji Wang <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR fixed the panic:
First of all, as mentioned in #9002, compute nodes must not panic on errors caused by UDF servers. cc @KveinAxel
Next, after #11839, decimal values are stored as strings in the arrow array. However, the panic message indicates that a float number with scientific notation
-5.10043e-05
was written by Python, and it couldn't be parsed by rust decimal. It turned out that user defined a function returningNUMERIC
, but the actual return value is afloat
rather thanDecimal
. Even it returns aDecimal
, its string form may still contains scientific notation and has a long precision number.When parsed by RisingWave, the tailing numbers and exponent will be truncated, resulting in wrong results.
So, this PR:
DECIMAL
and reject values other thanDecimal
type. (this problem doesn't exist in Java because it's strong typed)Checklist
./risedev check
(or alias,./risedev c
)Documentation