fix: bug preventing decimals in (-1, 1) from being inserted / queried #8720
Conversation
| @@ -357,6 +357,18 @@ | |||
| {"topic": "test_topic", "key": 1, "value": {"V0": 0.00}} | |||
| ] | |||
| }, | |||
| { | |||
| "name": "should insert decimals between zero and one", | |||
There was a problem hiding this comment.
nit: rest query translation tests take much longer to run than the standard QTT tests. instead it may make sense to just add a QTT test where we use a decimal literal that's 1 > x > -1
There was a problem hiding this comment.
Thank you! Will do that. I'm still figuring out how to run the feature tests selectively locally (this test is breaking anyway right now) -- will fix and update!
There was a problem hiding this comment.
Here are my configurations (the first runs all QTT tests, the second runs just a specific file). Also FYI there exists a decimal.json QTT test.
I think similar things work for RQTT
There was a problem hiding this comment.
@agavra Would it be ok for me to steal these images and toss them in a debugging.md file?:)
There was a problem hiding this comment.
@agavra Thank you, this is super helpful! I arrived at the same IntelliJ dialog after a bunch of messing around, but your setup is a lot cleaner!
There was a problem hiding this comment.
Interestingly the bug doesn't trigger on the query translation tests, only on the rest query translation tests. Leaving the rest-based test in here while I figure out exactly why the query translation tests don't trigger it...
maheshba
force-pushed
the
decimalbugfix
branch
from
d97f960
to
8c2003c
Compare
| for this edge case, we just take the scale and add one and use that for the precision instead. | ||
| */ | ||
| if (decimal.compareTo(BigDecimal.ZERO) == 0) { | ||
| /* We can't use BigDecimal.precision() directly for all cases, since it defines |
There was a problem hiding this comment.
Great documentation and explanation! If we ever decide to strictly follow the SQL standard for precision, it will be clear where to start from
maheshba
force-pushed
the
decimalbugfix
branch
from
8c2003c
to
5377032
Compare
maheshba
force-pushed
the
decimalbugfix
branch
from
5377032
to
ad50e80
Compare
maheshba
force-pushed
the
decimalbugfix
branch
from
ad50e80
to
d4e3e5a
Compare
Description
Fix for bug in #8413 where ksqldb doesn't allow non-zero decimals between (-0.1, 0.1) to be inserted or queried. The root cause is that we use BigDecimal to parse the string, and it has a different behavior for precision than the SQL standard for (-1, 1): "0.005" has precision 1 in BigDecimal, whereas SQL expects it to have precision 4 (total number of digits). For anything in (-0.1, 0.1) this causes the code to throw since scale is greater than precision.
Something to note: the fix introduces a quirk in how decimals starting with a period are processed: "0.005" and ".005" are both treated as having precision 4, whereas SQL expects precision 3 for the latter. To fix this we'd have to stop using BigDecimal as the internal representation (since it parses both strings identically) or extend it somehow. This doesn't change ksql behavior for the worse since right now both strings would fail.
Testing done
Added a unit test and a functional test; locally deployed and verified the case described in #8413.
Reviewer checklist