[bugfix] fix case-when issue #9702
Conversation
pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #9702 +/- ##
=============================================
+ Coverage 28.13% 70.01% +41.88%
- Complexity 53 5326 +5273
=============================================
Files 1936 1950 +14
Lines 103927 104332 +405
Branches 15770 15806 +36
=============================================
+ Hits 29235 73044 +43809
+ Misses 71832 26167 -45665
- Partials 2860 5121 +2261
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 |
| if (o != null) { | ||
| return o; | ||
| @ScalarFunction | ||
| public static Object caseWhen(boolean c1, Object o1, Object oe) { |
There was a problem hiding this comment.
Is the function name case or caseWhen?
There was a problem hiding this comment.
operator name is case, the relNode function name is caseWhen
There was a problem hiding this comment.
Interesting.. But does that mean these functions won't be recognized by v1 engine because the function name is case?
There was a problem hiding this comment.
not really. the info passed over to the stages contains SqlOperator and function. it will use SqlOperator to lookup first before using function name. thus v1 engine will correctly composed out the transform version of the "case"; and intermediate stage will use the scalar function correctly.
| for (int i = 0; i < numWhenStatements; i++) { | ||
| if (arguments.get(i * 2).getResultMetadata().getDataType() != DataType.BOOLEAN) { | ||
| hasLegacyFormat = true; | ||
| break; |
There was a problem hiding this comment.
(MAJOR) This can break when the first loop didn't break but the second loop break because some wrong arguments already added to the list.
Also, we should check in favor of the new order. We can check if the first half of the arguments are all boolean, and if the odd index arguments are all boolean. If both are all booleans or both are not all booleans or the first half are all booleans, use the new order; use the legacy one otherwise.
There was a problem hiding this comment.
got it. yeah also this is not in critical path i will just make sure the check occurs afterward by looping it again
There was a problem hiding this comment.
if (all_first_half_are_expression_boolean_type && odd_half_has_non_boolean_type) then use
Jackie-Jiang
added
backward-incompat
|
Please add some description in the PR to describe the backward incompatibility in certain case. Also, do not mark it as multistage |
walterddr
changed the title
|
WARNING: This PR is backward incompatible because we should not change the format sent by broker before upgrading servers. #10291 is the patch to revert back the broker format |
case when is not parsed the same in SqlNode and RelNode. this address the discrepancy in the compilation between v1 and v2.
Release Note
This change is backward incompatible with some of the
CASE WHENqueries.CASE WHEN <non_boolean_expr> THEN ...will not work during cluster upgrade,CASE WHEN CAST(<non_boolean_expr> AS BOOLEAN) THEN ...instead.CASE [WHEN <boolean_expr> THEN <boolean_expr>]+ ELSE <boolean_expr> ENDwill not work during cluster upgrade.