-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add lossy option to CastOptions for controlling precision loss in casts #9169
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
base: main
Are you sure you want to change the base?
Conversation
d94d8ce to
d25024b
Compare
Behavior matrix: - safe=true, lossy=false: Invalid casts return NULL, lossy casts return NULL - safe=true, lossy=true: Invalid casts return NULL, lossy casts allowed - safe=false, lossy=false: Invalid casts error, lossy casts error - safe=false, lossy=true: Invalid casts error, lossy casts allowed
d25024b to
e9ccb67
Compare
|
@alamb @scovich Please help review this when you're free, thanks. This wants to add It seems I can't add the |
|
I'm possibly missing some context here, but FWIW we already support lossy casts for decimals. I'm not sure about float-> int casting but I thought it rounded to zero, and only returnee null on overflow. |
As in, losing scale in e.g. |
|
Yes, truncation (i.e. precision loss) is permitted, overflow will error (or NULL). |
|
Sorry for the late reply, I missed this notification. Thanks for the reply。 This is raised in I found the Decimal-related tests(test_decimal_to_decimal_decrease_scale_and_precision_unchecked/test_decimal_to_decimal_increase_scale_and_precision_unchecked) in `arrow-cast/src/cast/mod.rs(Copied some test case below) test behavior for decimal castinput -> casted_result
Looking at |
|
Currently, |
yes, please let's not change how the cast kernels work. |
Which issue does this PR close?
What changes are included in this PR?
This PR adds
lossyinCastOptions, which aims to control precision loss in casts.Behavior matrix:
safelossytruefalsetruetruefalsefalsefalsetrueThe generated docs
Are these changes tested?
No tests, this added a new flag.
Are there any user-facing changes?
Yes, changed the
CastOptions