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 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. Heh, didn't like my comment, eh? :). I sometimes get a tad bit whimsical. Changed the comment, hopefully it looks better now. 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( Done 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 Done 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 Done 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 th Done 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: Done, but the line is > 90 characters, not sure what to do about that. Should I just ignore the warning? -- 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: Csaba Ringhofer <csringho...@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: Mon, 25 Mar 2024 21:57:11 +0000 Gerrit-HasComments: Yes
