-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[Branch-50] Backport: Support Decimal32/64 types (#17501) #17907
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
|
Why does this PR has substantially more lines that the original PR? Original PR: #17501
|
|
there are changes in |
* Support Decimal32/64 types * Fix bugs, tests, handle more aggregate functions and schema * Fill out more parts in expr,common and expr-common * Some stragglers and overlooked corners * Actually commit the avg_distinct support --------- Co-authored-by: Andrew Lamb <[email protected]>
3bb79ae to
d8b844d
Compare
|
Ok now it seems better, its not a perfect match because some of the changes were based upon #17459, which wasn't backported. |
alamb
left a comment
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.
Thanks @AdamGS -- I worry that this is not semver compatible...
If you need this code earlier than Nov maybe we could accelerate the timeline of 51.0.0 but not take everything on main 🤔
| /// 64bit float | ||
| Float64(Option<f64>), | ||
| /// 32bit decimal, using the i32 to represent the decimal, precision scale | ||
| Decimal32(Option<i32>, u8, i8), |
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 🤔 this is technically a breaking API change I think because ScalarValue is a public enum and not parsed non_exhastive... So we can't backport it to the branch-50 without breaking the API
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.
That makes perfect sense, should've noticed it myself.
I can wait until November, sorry about causing so much noise pushing for this release, at least we'll get the docs fix in.


Which issue does this PR close?
50.2.0(minor) #17849Rationale for this change
Backports initial support for decimal 32/64 to the 50.x branch
What changes are included in this PR?
See original PR:
Are these changes tested?
Includes some tests and reinforced by SLT. Follow up PR contains more tests.
Are there any user-facing changes?
No