-
Notifications
You must be signed in to change notification settings - Fork 281
Handling mysql decimal data types with precision 19 or higher #6369
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
Signed-off-by: Divyansh Bokadia <[email protected]>
oeyh
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 for the fix! Were you able to verify the changes with export and stream data?
|
|
||
| if (value instanceof Map) { | ||
| Object data = ((Map<?, ?>)value).get(BYTES_KEY); | ||
| if (data instanceof byte[]) { |
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.
Is data always of byte[] type? Do we need an else block here?
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.
Did not encounter the Map use case. In my tests the values were an ArrayList type
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 we should still have the else condition to log this case if it ever happens. Otherwise you will get the exception on line 139, which would make it seem that we didn't find the right type.
Yes, I was able to verify the changes. |
| Arguments.of(MySQLDataType.BIT, "bit_col", Map.of("bytes", new byte[]{ 1, 2, 3, 4 }), new BigInteger("16909060")), // Direct BigInteger interprets the bytes in big-endian order. | ||
|
|
||
| //DECIMAL tests represented as byte arrays | ||
| Arguments.of(MySQLDataType.DECIMAL, "decimal_col", new ArrayList<>(List.of(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16)), new BigDecimal("1339673755198158349044581307228491536")), |
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.
Could be worth testing negative and fractional values as well, converted to BigDecimal?
|
|
||
| private Number handleByteArray(final Object value) { | ||
| if (value instanceof byte[]) { | ||
| return new BigDecimal(new BigInteger((byte[]) value)); |
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.
Each of these conditions at some point calls this: new BigDecimal(new BigInteger(someBytes). Refactor this out into a method so that changes to one affect others.
|
|
||
| if (value instanceof Map) { | ||
| Object data = ((Map<?, ?>)value).get(BYTES_KEY); | ||
| if (data instanceof byte[]) { |
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 we should still have the else condition to log this case if it ever happens. Otherwise you will get the exception on line 139, which would make it seem that we didn't find the right type.
Description
Adds support for FIXED_LEN_BYTE_ARRAY encoding (ArrayList) in NumericTypeHandler to handle MySQL DECIMAL values ≥2^63. Previously caused pipeline failures when processing RDS snapshot exports containing high-precision decimal values.
Fixes issue where RDS exports encode large DECIMAL values as ArrayList instead of primitive types.
Issues Resolved
Resolves #6339
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.