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 7: Code-Review+1 (2 comments) Rebuilt PS7 in a test cluster and compared both planners over about 40 boolean-vs-numeric forms: literals, columns, decimal(38,7)/float/double, NULL operands, casts, IN and BETWEEN. Everything agrees, and the FLOAT choice for decimal matches what the original planner does - its analyzed query is CAST(d AS FLOAT) = CAST(b AS FLOAT). Same set on the unpatched tree: 2 = true came back true, -1 = true came back true, "where 2 = bool_col" returned 4, and decimal > boolean plus "x BETWEEN 0 AND 2" failed at validation. So BETWEEN gets fixed along the way; nothing covers it in the test file if you want a case for it. Also ran test_calcite_planner.py (calcite.test, calcite_subquery, cte) in report mode before and after the patch: the only difference is the case this patch adds, no new failures. One thing that stays different, and not from this patch: IN with a boolean. "1 in (true, false)" is true on the original planner and fails with "Values passed to IN operator must have compatible types" on Calcite, same on the unpatched tree. http://gerrit.cloudera.org:8080/#/c/24217/7/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/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java@121 PS7, Line 121: // IMPALA-14911: When there is an equality with one side boolean The method returns false for everything now, so the decimal wording here reads narrower than what the code does - same for the paragraph in the commit message. Maybe lead with the literal replacement, since that is what makes "2 = true" diverge, and keep decimal as the second reason? http://gerrit.cloudera.org:8080/#/c/24217/7/testdata/workloads/functional-query/queries/QueryTest/binary_exprs_with_boolean.test File testdata/workloads/functional-query/queries/QueryTest/binary_exprs_with_boolean.test: http://gerrit.cloudera.org:8080/#/c/24217/7/testdata/workloads/functional-query/queries/QueryTest/binary_exprs_with_boolean.test@324 PS7, Line 324: select binary_col = bool_col as a from test_bool_binary_eq order by a; This is the "=" case again (line 178) - I think the ">" variant was meant here. "binary_col > bool_col" gives the same "operands of type BINARY and BOOLEAN are not comparable" on the original planner, so it drops in as is. -- 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: 7 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 18:16:41 +0000 Gerrit-HasComments: Yes
