Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22870 )
Change subject: IMPALA-14061: Calcite Planner: added Calcite rules ...................................................................... Patch Set 4: (6 comments) This is my initial review while waiting for https://gerrit.cloudera.org/c/22864 to merge. http://gerrit.cloudera.org:8080/#/c/22870/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22870/4//COMMIT_MSG@38 PS4, Line 38: Calcite has multiple places where it : creates a SEARCH operator via "simplifying" the RexNodes within : various rules. This operator is not supported directly in Impala : and we need to call "expandSearch" to handle this. What is Calcite SEARCH operator map to Impala? Is it BETWEEN operator? http://gerrit.cloudera.org:8080/#/c/22870/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java: http://gerrit.cloudera.org:8080/#/c/22870/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/coercenodes/CoerceNodes.java@384 PS4, Line 384: 100 What is this 100 constant means? http://gerrit.cloudera.org:8080/#/c/22870/4/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/22870/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java@28 PS4, Line 28: import java.util.List; : /** nit: Separate with new line. http://gerrit.cloudera.org:8080/#/c/22870/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/operators/ImpalaRexBuilder.java@42 PS4, Line 42: somethign nit: something http://gerrit.cloudera.org:8080/#/c/22870/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java: http://gerrit.cloudera.org:8080/#/c/22870/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java@93 PS4, Line 93: timeline_.markEvent("Run expand nodes"); Timeline event usually marked at the end of step rather than at the beginning. Event names are in the past tense, like following: Query Timeline Query submitted: 44.37us (44370) Planning finished: 41ms (41633619) CatalogDdlRequest finished: 233ms (233902721) Applied catalog updates from DDL: 247ms (247352880) Rows available: 247ms (247520762) Unregister query: 344ms (344975468) http://gerrit.cloudera.org:8080/#/c/22870/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/LogUtil.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/util/LogUtil.java: http://gerrit.cloudera.org:8080/#/c/22870/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/LogUtil.java@33 PS4, Line 33: public static void logDebug(Object resultObject, String planString) { Can return early if LOG.isDebugEnabled() is false. -- To view, visit http://gerrit.cloudera.org:8080/22870 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6671f7ed298a18965ef0b7a5fc10f4912333a52b Gerrit-Change-Number: 22870 Gerrit-PatchSet: 4 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Anonymous Coward (816) Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Tue, 27 May 2025 17:42:07 +0000 Gerrit-HasComments: Yes
