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
Fix Data-Correctness Bug in GTE Comparison in BinaryOperatorTransformFunction #9461
Conversation
.../java/org/apache/pinot/core/operator/transform/function/BinaryOperatorTransformFunction.java
Show resolved
Hide resolved
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.
Good catch! You may add a test case in BinaryOperatorTransformFunctionTest
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.
LGTM. +1 on adding a unit test for this.
Added a UT. Existing tests weren't able to catch this case since this only happens when the argument types are:
I also found a bug in my PR. I had only updated calls for one of the |
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.
Good catch!
Codecov Report
@@ Coverage Diff @@
## master #9461 +/- ##
============================================
- Coverage 69.88% 69.83% -0.05%
- Complexity 5127 5134 +7
============================================
Files 1909 1910 +1
Lines 101749 101769 +20
Branches 15440 15444 +4
============================================
- Hits 71104 71071 -33
- Misses 25627 25677 +50
- Partials 5018 5021 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
…Function (apache#9461) * Fix Bug in Handling GTE Comparison in BinaryOperatorTransformFunction * Add UT * Fix bug and add another test
fixes #9460
The BinaryOperatorTransformFunction::compare methods already call
getIntResult
, but the places where it's called callsgetIntResult
again.https://github.com/apache/pinot/blob/master/pinot-core/src/main/java/org/apache/pinot/core/operator/transform/function/BinaryOperatorTransformFunction.java#L371
Say the comparison is: 10 >= 20
Then, the call chain is (bottom-up): Double.compare(10, 20) ==> -1 ==> getIntResult(-1) ==> 0 ==> getIntResult(0) ==> 1
Which means 10 >= 20 gets evaluated to true
cc: @yupeng9 @chenboat