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 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/24217/2/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/2/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java@123 PS2, Line 123: if (SqlTypeUtil.isBoolean(type1) && SqlTypeUtil.isNumeric(type2)) { > Hmmm... You are right, and my example does not hold. On the original planner both "d1 > b" and "d1 > sleep(100)" analyze fine and resolve to gt(INT, INT), and your two-constant case analyzes fine here too. I read DefaultCompatibility and assumed BinaryPredicate consulted that matrix, but the overload search in getBuiltinFunction never asks it. Sorry for the noise. While checking I ran the comparisons through both planners on PS2, frontend only, synthetic catalog, and one thing did come out of it: - si > b and si > sleep(100): fixed by the patch. The boolean arrives as CASE(IS NOT NULL($0), CAST(CASE($0, 1:TINYINT, 0:TINYINT)):SMALLINT, null:SMALLINT). - d1 > b, d1 > sleep(100), cast(1.1 as decimal(2,1)) > true, dbl > b: AssertionError from `assert SqlTypeUtil.canCastFrom(toType, fromType, mappingRule)` in AbstractTypeCoercion.needToCast, line 309 of Calcite 1.42, reached through ImpalaTypeCoercionImpl.binaryComparisonCoercion. Without the patch those four stop at validation with "Cannot apply '>' to arguments of type '<DECIMAL(9, 0)> > <BOOLEAN>'". So SqlTypeUtil.isNumeric() looks wider than the set Calcite will cast a boolean to: smallint goes through, DECIMAL and DOUBLE hit that assert. The assert itself only fires because the test JVM runs with assertions on; in an impalad the coercion would build the cast anyway, and I have not traced what happens after that. Would narrowing the override to the exact integer types work? That keeps what the patch fixes and leaves decimal and double with the validation error instead of an internal one. -- 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: 2 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: Sun, 23 Aug 2026 20:30:07 +0000 Gerrit-HasComments: Yes
