-
Notifications
You must be signed in to change notification settings - Fork 278
bug: Fix string decimal type throw right exception #3248
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?
bug: Fix string decimal type throw right exception #3248
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3248 +/- ##
============================================
+ Coverage 56.12% 60.07% +3.94%
- Complexity 976 1426 +450
============================================
Files 119 172 +53
Lines 11743 15926 +4183
Branches 2251 2631 +380
============================================
+ Hits 6591 9567 +2976
- Misses 4012 5031 +1019
- Partials 1140 1328 +188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Root cause for test failures :
|
de1618a to
a9b9ccd
Compare
| trimmed, | ||
| input_str, | ||
| "STRING", | ||
| &format!("DECIMAL({},{})", precision, scale), |
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.
do we need to allocate this string here with the format!? It looks like this string is only used when an error occurs?
Can you share benchmarks showing before/after these changes?
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.
Sure @andygrove
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.
We don't currently hve string to decimal cast benchmarks at the moment. Let me create the benchmarks (based on string to int casting inputs) and rerun the benchmarks before and after string allocation fix
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.
Thank you . I was able to fix the unwanted allocation and improved performance by 2x
|
Thank you for the suggestion @andygrove . These are the benchmarks before and after removing string allocations even for happy paths |
Which issue does this PR close?
Closes #.
Rationale for this change
What changes are included in this PR?
How are these changes tested?