Fix null literal handling for null intolerant functions in multi-stage query engine #13255
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #13255 +/- ##
=============================================
- Coverage 61.75% 35.27% -26.48%
+ Complexity 207 6 -201
=============================================
Files 2436 2459 +23
Lines 133233 135356 +2123
Branches 20636 20987 +351
=============================================
- Hits 82274 47750 -34524
- Misses 44911 84091 +39180
+ Partials 6048 3515 -2533
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| } | ||
|
|
||
| @Test | ||
| public void testNullLiteralSelectionInV2() throws Exception { |
There was a problem hiding this comment.
Can we rewrite these tests as one method per test or, maybe better, one provider with the query and the expected results?
We have tons of tests like this (or the one above) where the test has a lot of assertions. That is an anti-pattern we should try to get rid of. We never have time to modify the old code, but we should try to write new tests in a better way.
Just to be clear, there are two main advantages of the suggested approach:
- The tests are easier to read (each test is a query and an expected result)
- In case some test fail, the following tests are executed. In the current version if test 2 fails, 3, 4, etc are not executed, which means it can be more difficult to find the actual problem (or to know if there are more than one!)
There was a problem hiding this comment.
Makes sense, I've added a data provider and updated both these tests. Let me know if this is along the lines of what you were thinking about. We could very easily add another parameter for v1 / v2 and use that to determine the query engine and the expected number of rows in order to combine both the test methods into a single one. However, given the notable difference in behavior between the two engines in this case (v1 simply handles it in the broker and returns a single result), I preferred keeping them as separate tests using the same data provider to emphasize the difference.
…e query engine (apache#13255) * Fix null literal handling for null intolerant functions in multi-stage query engine * Use data provider for null literal selection integration tests
SELECT add(null, 1) FROM mytablefail with the multi-stage query engine returning the following exception:HepPlannerfires the PinotEvaluateLiteralRule.DOUBLE NOT NULLbecause of the primitive return type of the method (see here / here). This causes the type mismatch issue in thePinotEvaluateLiteralRulebecause the new project created here has aRexLiteralwith typeDOUBLEand valuenull, whereas the old project has aRexCallwith typeDOUBLE NOT NULL.nullbecause they are "null intolerant" andnullwill be returned without even invoking the method if any of the arguments is null (here).Strict(function returns null if and only if one of the arguments are null) andSemiStrict(function returns null if one of the arguments is null, and possibly other times) annotations can help us out here - https://github.com/apache/calcite/blob/694b556a2ece4953d8e9145352eb2340e1fac908/core/src/main/java/org/apache/calcite/schema/impl/ScalarFunctionImpl.java#L185-L205. With this, the function's return type in Calcite will beDOUBLEinstead ofDOUBLE NOT NULLand thePinotEvaluateLiteralRuleworks out fine.NullHandlingIntegrationTest(to support null handling in the multi-stage query engine) and enables two additional tests to run on both query engines.bugfix