Joe McDonnell 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 12: (6 comments) http://gerrit.cloudera.org:8080/#/c/21109/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java: http://gerrit.cloudera.org:8080/#/c/21109/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java@70 PS12, Line 70: // O BENEVOLENT REVIEWER AND CODE INSPECTOR... : // TODO: Please hold off on reviewing this file. I held off on cleaning this up until : // this gets past the experimental stage. Some of the code in SingleNodePlanner : // is duplicated here, so this will involve a general rewrite. After more Calcite : // code gets committed and the planner works for a good portion of the queries, this : // will get rewritten into its final form. Let's rework this comment since we're looking to merge this. I would describe what this thing does (i.e. it takes a single-node PlanNode tree from Calcite and produces a TExecRequest), describe what the corresponding code on the regular Impala planner is. Then, I would say that this needs refactoring as we get farther along and maybe give some general idea of where we want to end up. http://gerrit.cloudera.org:8080/#/c/21109/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java@101 PS12, Line 101: * Create an exec request for Impala to execute based on the supplied plan. Can you add a comment that this is similar to Frontend's createExecRequest()? http://gerrit.cloudera.org:8080/#/c/21109/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java@168 PS12, Line 168: Hive CBO Nit: Since we're using Calcite, let's update locations that reference Hive CBO to say Calcite. http://gerrit.cloudera.org:8080/#/c/21109/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java@196 PS12, Line 196: * @param planNodeRoot root node of the Impala physical plan : * @param destination path to the target table if the table is not null : * @param isOverwrite true if it is an INSERT OVERWRITE statement : * @param writeId write ID of the target table if the table is not null : * @return list of plan fragments in the order [root fragment, child of root ... : * leaf fragment] : * @throws ImpalaException : * @throws HiveException Update this to match the new signature or remove it http://gerrit.cloudera.org:8080/#/c/21109/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/ExecRequestCreator.java@335 PS12, Line 335: * Return true if any join in the plan rooted at 'root' was inverted. : * : * TODO: This should be replaced once we conclude the changes contained in this method : * are safe to be pushed to Planner.invertJoins, i.e., they do not cause any : * performance regressions with Impala FE. Could you add a sentence or two about what is different? From looking at this, it is a slightly different signature (returning bool, taking Analyzer), and calling computeStats() in a couple place where we currently don't. http://gerrit.cloudera.org:8080/#/c/21109/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/validate/ImpalaConformance.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/validate/ImpalaConformance.java: http://gerrit.cloudera.org:8080/#/c/21109/12/java/calcite-planner/src/main/java/org/apache/impala/calcite/validate/ImpalaConformance.java@25 PS12, Line 25: https://javadoc.io/doc/org.apache.calcite/calcite-core/latest/index.html This might be a better link: https://calcite.apache.org/javadocAggregate/org/apache/calcite/sql/validate/SqlConformance.html -- 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: 12 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: Mon, 25 Mar 2024 20:48:12 +0000 Gerrit-HasComments: Yes
