Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22408 )
Change subject: IMPALA-13520: Support in clause coercing ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/22408/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22408/7//COMMIT_MSG@11 PS7, Line 11: we do have > nit: By 'we' I suppose you mean the Impala planner ? It wasn't clear from Done http://gerrit.cloudera.org:8080/#/c/22408/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/CombineValueNodesRule.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/CombineValueNodesRule.java: http://gerrit.cloudera.org:8080/#/c/22408/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/CombineValueNodesRule.java@41 PS7, Line 41: * This is needed because there is a test case in test_exprs.py which creates : * an IN expression with 10000 literals in the IN clause. Calcite will create : * 10000 RelNodes for this which becomes incredibly slow at execution time. : * > I think it is better to keep the comment general enough and not point to a Done http://gerrit.cloudera.org:8080/#/c/22408/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/rules/CombineValueNodesRule.java@48 PS7, Line 48: CombineValueNodesRule > nit: can we name this CombineValuesNodesRule (with the plural 'Values' sinc Done http://gerrit.cloudera.org:8080/#/c/22408/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/22408/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java@80 PS7, Line 80: // Only handle IN clause with a list as its second param > Is a NOT IN list handled differently ? For the same example where we have a NOT IN also comes into the method. http://gerrit.cloudera.org:8080/#/c/22408/7/java/calcite-planner/src/main/java/org/apache/impala/calcite/type/ImpalaTypeCoercionImpl.java@93 PS7, Line 93: for (SqlNode node : inList) { : rightOperandTypes.add(deriveType(validator, scope, node)); : } : > I think we only need to store the unique operand types into the rightOperan It is true that we only need the unique types when calling the getCompatibleType. But this list is also passed into the coerceInList method below where we iterate through each fromType. -- To view, visit http://gerrit.cloudera.org:8080/22408 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I492845d623766b9182bca5eeca22eb3352ef2f3d Gerrit-Change-Number: 22408 Gerrit-PatchSet: 7 Gerrit-Owner: Steve Carlin <scar...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Michael Smith <michael.sm...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Thu, 30 Jan 2025 19:53:15 +0000 Gerrit-HasComments: Yes
