Fix Float and Decimal coercion #14273
Conversation
findepi
force-pushed
the
findepi/fix-float-and-decimal-coercion-b58e78
branch
from
4e8e850 to
0f69629
Compare
findepi
force-pushed
the
findepi/fix-float-and-decimal-coercion-b58e78
branch
from
0f69629 to
056e2d0
Compare
findepi
force-pushed
the
findepi/fix-float-and-decimal-coercion-b58e78
branch
from
056e2d0 to
52f19ee
Compare
There was a problem hiding this comment.
Thanks @findepi -- this area is a bit out of my league here to understand the repercussions of this change
@jonahgao @berkaysynnada and @ozankabak and I have been discussing coercion related to @nuno-faria s (very) recent change here:
Perhaps they can weigh in here. I feel like there is a tension here and that we are in serious danger of the behavior oscillating 🤔
I feel like in some past version of DataFusion we used to coerce to Floats and that was changed to Decimal for some reason
Maybe what we can do is write down
- What the current coercion behavior is
- What we want it to be (and why we want to change it)
- Get agreement on to goal
- Then make the code match the agreement
Before the change, Float* and Decimal* would coerce to a decimal type. However, decimal cannot store all the float values: different range, NaN, and infinity. Coercing to floating point is more desirable and also what other systems typically do.
findepi
force-pushed
the
findepi/fix-float-and-decimal-coercion-b58e78
branch
from
52f19ee to
bed8a43
Compare
|
@alamb thanks for your feedback. I agree it's important to avoid back-and-forth with a change, so the broader review the better.
I don't think this particular change should be controversial though. See issue, but also:
let's do it! |
Which issue does this PR close?
Closes #14272
Rationale for this change
Before the change, Float* and Decimal* would coerce to a decimal type. However, decimal cannot store all the float values: different range, NaN, and infinity. Coercing to floating point is more desirable and also what other systems typically do.
What changes are included in this PR?
Numeric coercion rules change
Are these changes tested?
yes
Are there any user-facing changes?
yes