-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Store Decimal precision and size while normalising #15785
Conversation
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Signed-off-by: Manan Gupta <manan@planetscale.com>
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 adding the size/scale to the bind var replacement is a good idea. Let's simplify the calculation of the values though.
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
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.
Looking bueno. Thanks manan!
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Signed-off-by: Manan Gupta <manan@planetscale.com>
Awesome!!! Thank you everyone for the reviews! The last commit adds the complete Q14 to the test suite verifying that the query works now! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15785 +/- ##
==========================================
- Coverage 68.44% 68.43% -0.01%
==========================================
Files 1558 1558
Lines 195822 195928 +106
==========================================
+ Hits 134032 134087 +55
- Misses 61790 61841 +51 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com> Co-authored-by: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Co-authored-by: Dirkjan Bussink <d.bussink@gmail.com>
Description
This PR fixes part of the issue described in #15784.
As described in #15784 (comment), the problem arises because when Vtgates normalize the decimal values, they don't store the scale and length. This PR makes this change by adding 2 more fields to the Arguments struct.
This also affects the caching of queries. Since the precision of
100.01
and100.0001
are different, queries that only differ in this literal will actually have different results and shouldn't be cached to the same plan.In order to find the size and precision of the decimal value, we are relying on the existing code in
decimal
package to parse the decimal value and return the exponent and the size.https://dev.mysql.com/doc/refman/8.3/en/precision-math-decimal-characteristics.html page talks about decimal value and its handling of length and precision
Related Issue(s)
CASE
and JOIN bind variables #15787 being merged, this PR resolves the remaining issue in Bug Report: TPCH query returns incorrect precision count in the fields #15784.Checklist
Deployment Notes