Corrected scale representation for NUMERIC/DECIMAL #251
Conversation
|
Awesome, thanks a lot! I'm at conference today, but I'm going to review
this asap.
|
|
Any updates on merging this PR? This is a blocker for us. |
|
I am sorry for the delay, Gunnar is away this week. I'll take look at it tomrrow. Is there any chance you could provide integration test for the functionality. Also just a reminder - having the issue merged does not mean that we will release the new version with the fix - the release date for 0.5.2 has not been set yet. |
|
@user453584 @jpechane I am on vacation till 27 of July, anyway I'll try to provide integration tests in the next days. |
|
@jpechane I have added integration test for the fix and verified that it correctly check the scenario fixed with this PR, so it fails with the actual master and passes with this branch. |
There was a problem hiding this comment.
Hey @mcapitanio, the change looks good overall, I've added some comments inline about some questions. Could you also squash all the commits into one and add the issue number to the commit message ("DBZ-287 Corrected scale..."). I can also help with the latter if needed. Thanks!
| return newDecimal; | ||
| } | ||
| if (column.scale() > -1) { | ||
| String decimalWithTrailingZeros = String.format("%-" + column.scale() + "s", newDecimal.doubleValue()) |
There was a problem hiding this comment.
Why is it that you create this string and then instantiate a new BigDecimal with it? Shouldn't it be enough to do newDecimal = newDecimal.setScale(column.scale())?
There was a problem hiding this comment.
I found that simply changing the scale does not affect the string serialized value of the BigDecimal, so it does not recover the missing trailing zeros.
Anyway I found maybe a more elegant solution using the MathContext instead of the string:
MathContext mc = new MathContext((newDecimal.precision() - newDecimal.scale()) + column.scale());
newDecimal = new BigDecimal(newDecimal.doubleValue(), mc);
So in the case of the integration test, where I have a DECIMAL(3,2) and a database value of 1.1:
newDecimal.precision() assumes the value of 2
newDecimal.scale() assumes the wrong value of 1
newDecimal.precision() - newDecimal.scale() is the size of the integer part of the original newDecimal (so it would assume the value 1)
column.scale() is the correct scale for the newDecimal, so it would be 2
The MathContext would consider a precision of 3 and the value will be correctly represented as 1.10.
| if (newDecimal == null) { | ||
| return newDecimal; | ||
| } | ||
| if (column.scale() > -1) { |
There was a problem hiding this comment.
How about checking for if (column.scale() > newDecimal.scale())? I.e. not re-scale if the value already has the right scale.
There was a problem hiding this comment.
@gunnarmorling Totally agree! Thanks for the suggestion!
|
In terms of a release, I would think that we can ship a 0.5.2 some time next week, including this change and hopefully a few others from the pending PRs. They warrant another bugfix release for sure. |
|
🙌 |
|
@gunnarmorling I've squashed all my commits in just one commit, hope it is as you expected. I found a different solution for the fixing, not using the string but with the more elegant usage of MathContext: just changing the scale to the existing decimal value unfortunately does not solve the problem. |
Hum, did you re-assign the |
|
Rebased and applied. Thanks a lot, @mcapitanio! Note that I changed the implementation to use |
|
@gunnarmorling Absolutely, the setScale is the best solution, I didn't realize it was necessary to make the re-assignement! Hope in helping Debezium again, Postgres connector is so crucial for us! |
|
Awesome, your contributions are always welcome! A release for shipping this
fix will be done soon.
|
|
@gunnarmorling Thanks for helping this issue along (and @mcapitanio for the PR!). @gunnarmorling What is the status of the 0.5.2 release? This bug has been a blocker for us, so we're hoping to get the new release pulled down as soon as possible |
|
Hi, the 0.5.2 release is scheduled for next week. If you like, you can download the latest snaphot build from here: https://oss.sonatype.org/content/repositories/snapshots/io/debezium/. |
@gunnarmorling This PR is a proposal to fix the issue for the missing trailing zeros in the double representation of a NUMERIC/DECIMAL data type in PostgreSql.
The bug is reported in the Issue DBZ-287.