Steve Carlin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21109 )
Change subject: IMPALA-12872: Use Calcite for optimization - part 1: simple queries ...................................................................... Patch Set 5: (8 comments) http://gerrit.cloudera.org:8080/#/c/21109/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21109/5//COMMIT_MSG@7 PS5, Line 7: IMPALA-12872: Use Calcite for ... > nit: combine the two lines. it seems short enough. How about 'Use Calcite Done http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java File java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java: http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java@33 PS5, Line 33: analyzer > nit:use underscore to be consistent Removed this since it is not needed. The analyzer is in the PlannerContext http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java@36 PS5, Line 36: ctx > nit: use underscore to be consistent with other variable names. Done http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/rel/node/ParentPlanRelContext.java@39 PS5, Line 39: public final Class parentClass_; > We should use the actual base class name (RelNode/ImpalaPlanRel) instead of Removed this since it is not needed in this cut http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java File java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java: http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteDb.java@31 PS5, Line 31: tableMap > nit: use underscore Done http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java File java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java: http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/schema/CalciteTable.java@78 PS5, Line 78: private List<RelReferentialConstraint> foreignKeys; > This field is not used in this implementation. Is it for future use ? Removed it since not used in this cut http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java File java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java: http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/service/CalciteOptimizer.java@87 PS5, Line 87: LOG.debug(getDebugString(resultObject)); > nit: should this logging be in an else block ? Good catch I put a return in the if block. Also did this in other Calcite* files http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/util/NotSupported.java File java/experimental-planner/src/main/java/org/apache/impala/calcite/util/NotSupported.java: http://gerrit.cloudera.org:8080/#/c/21109/5/java/experimental-planner/src/main/java/org/apache/impala/calcite/util/NotSupported.java@33 PS5, Line 33: > nit: missing 'are' . Here and subsequent messages. Done -- To view, visit http://gerrit.cloudera.org:8080/21109 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I453fd75b7b705f4d7de1ed73c3e24cafad0b8c98 Gerrit-Change-Number: 21109 Gerrit-PatchSet: 5 Gerrit-Owner: Steve Carlin <scar...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Wed, 20 Mar 2024 19:50:45 +0000 Gerrit-HasComments: Yes
