Aleksandr Efimov has posted comments on this change. ( http://gerrit.cloudera.org:8080/24217 )
Change subject: IMPALA-14911: Calcite planner: Fix boolean to numeric comparison ...................................................................... Patch Set 6: (3 comments) Rebuilt PS6 in a test cluster and ran the boolean comparisons through both planners. Three notes below. http://gerrit.cloudera.org:8080/#/c/24217/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java: http://gerrit.cloudera.org:8080/#/c/24217/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java@85 PS6, Line 85: public RexNode makeCast( This catches every cast whose operand is boolean, while the Calcite code it replaces only special-cases BOOLEAN -> exact numeric (makeCastBooleanToExact); other targets already end up in makeAbstractCast. Would narrowing it to SqlTypeUtil.isExactNumeric(type) keep makeCast's literal handling for the rest? I did not find a query where it matters, so this is a question, not a finding. Also missing @Override - the signature does match Calcite 1.42, so it overrides today, but the annotation would catch a future signature change. http://gerrit.cloudera.org:8080/#/c/24217/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java: http://gerrit.cloudera.org:8080/#/c/24217/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java@119 PS6, Line 119: protected boolean booleanEquality(SqlCallBinding binding, While checking the decimal path I ran into the integer one, which still differs from the original planner: select 2 = true calcite: true original: false select -1 = true calcite: true original: false select count(*) from functional.alltypestiny where 2 = bool_col calcite: 4 original: 0 Calcite's booleanEquality Case1 replaces a non-zero numeric literal with TRUE rather than casting the boolean, so "2 = bool_col" becomes "TRUE = bool_col". Decimal no longer goes through it after this patch, and every column-to-column case in the new test file agrees between planners - it is only a numeric literal on one side. Since booleanEquality is already overridden here, is closing the literal case cheap enough to do now, or would you rather keep it for its own Jira? Results diverge silently, which is why I bring it up. http://gerrit.cloudera.org:8080/#/c/24217/6/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java@137 PS6, Line 137: public RelDataType commonTypeForBinaryComparison( Small one: no @Override here, and the base version treats both parameters as nullable - it returns null for them before anything else, while this one calls SqlTypeUtil.isBoolean straight away. I could not reach a null from SQL, so it may well be theoretical. -- To view, visit http://gerrit.cloudera.org:8080/24217 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I764dcbd2fd0d1323bfb964ed91d94cd3def594c0 Gerrit-Change-Number: 24217 Gerrit-PatchSet: 6 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 25 Aug 2026 16:13:40 +0000 Gerrit-HasComments: Yes
