Fix decimal decoding #401
Fix decimal decoding #401
Conversation
|
/run-all-tests tikv=release-2.0 tidb=release-2.0 pd=release-2.0 |
Codecov Report
@@ Coverage Diff @@
## master #401 +/- ##
=========================================
Coverage ? 55.14%
=========================================
Files ? 127
Lines ? 5812
Branches ? 864
=========================================
Hits ? 3205
Misses ? 2313
Partials ? 294
Continue to review full report at Codecov.
|
| this.digitsInt = wordsInt * digitsPerWord + leadingDigits; | ||
| this.digitsFrac = wordsFrac * digitsPerWord + trailingDigits; | ||
| this.digitsInt = (byte)(wordsInt * digitsPerWord + leadingDigits); | ||
| this.digitsFrac = (byte)(wordsFrac * digitsPerWord + trailingDigits); |
There was a problem hiding this comment.
type conversion seems unnecessary. Digits of int and fraction can never go beyond 127. tidb does this simply because it is static type language. It has to do such conversion otherwise compiler will not be happy.
There was a problem hiding this comment.
To keep the same behavior as in TiDB which does a cap.
| int x = 0; | ||
| switch (size) { | ||
| case 1: | ||
| x = b[start]; | ||
| x = (byte)b[start]; |
There was a problem hiding this comment.
In java byte is signed. b[start] is unsigned in fact since we read as unsigned byte.
So actually convert to byte will cause it to be signed. It's not trivial.
| break; | ||
| case 3: | ||
| int sign = b[start] & 128; | ||
| if (sign > 0) { | ||
| x = 255 << 24 | (b[start] & 0xFF) << 16 | (b[start + 1] & 0xFF) << 8 | (b[start + 2]); |
There was a problem hiding this comment.
Just a reminder: b's type is byte[] which shares a range from -128 to 127 whereas golang's byte shares range from 0 to 255. We are better take and operation with 0xFF.
There was a problem hiding this comment.
Additionally, 255 << 24 could be overflow. We need convert 255 to a uint and then convert back to int.
There was a problem hiding this comment.
my bad, java does not have uint..
There was a problem hiding this comment.
~(0x7F << 24 | b[start] << 16 | b[start + 1] << 8 | (b[start + 2]) + 1);
There was a problem hiding this comment.
you may use ~(((b[start] & 7FFF) << 16 | (b[start + 1] & 0xFF) << 8 | (b[start + 2] & 0xFF)) + 1);
There was a problem hiding this comment.
- b's type is int not byte[].
- 255 as const literal is int type.
There was a problem hiding this comment.
255 << 24 has correct binary form. Numeric overflow does not affect binary. ignore my comment.
|
We are better to add some comments for |
|
/run-all-tests tikv=release-2.0 tidb=release-2.0 pd=release-2.0 |
Old implementation was wrong dealing with unsigned / signed part of Java and some code path might break in specific values.
This change is