Aman Sinha 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 17: (14 comments) http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java: http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/rel/util/CreateExprVisitor.java@1 PS17, Line 1: /* In some files, the License header is enclosed in the multi statement syntax /*. .. */ whereas in others it is the single line comment. Pls make it consistent in all the files and use whatever is being used in existing Impala files. Impala uses the single line comment style. '// ' http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java: http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java@30 PS17, Line 30: queryCtx; Pls add underscore http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/schema/ImpalaCalciteCatalogReader.java@31 PS17, Line 31: stmtTableCache Add underscore http://gerrit.cloudera.org:8080/#/c/21109/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java: http://gerrit.cloudera.org:8080/#/c/21109/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@94 PS16, Line 94: : nit: add space or newline after : so that the query string is separated out. http://gerrit.cloudera.org:8080/#/c/21109/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@148 PS16, Line 148: if (e == null) { > I wanted the ability to print the stack trace hree. But I also throw an In So this should be e != null then. http://gerrit.cloudera.org:8080/#/c/21109/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@166 PS16, Line 166: if (line.trim().startsWith("--") || line.trim().equals("")) { Comments can be multi-statement also e.g. /* ignore this */ SELECT a, b, c. There could be other patterns not handled here. Can we not use the parser methods directly ? If this is supposed to be a temporary function or if this is expected to go through revision later, pls add the relevant comments. http://gerrit.cloudera.org:8080/#/c/21109/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@209 PS16, Line 209: LOG.info("Using Impala planner"); Can you make this consistent with the corresponding message for Calcite planner on line 94. Something like: LOG.info("Using Impala Planner for the following query: " + queryCtx.getStmt()); http://gerrit.cloudera.org:8080/#/c/21109/16/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@245 PS16, Line 245: Frontend Timeline (Calcite Planner)") This QueryContext is used in both cases right ? i.e if Calcite planner fails, fall back to Impala planner. So this message should be modified if the fall back occurs. http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java: http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@116 PS17, Line 116: logical plan nit: to be precise, can we say 'initial logical plan' http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@121 PS17, Line 121: optimized plan nit: to be precise, can we say 'optimized logical plan' . http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@234 PS17, Line 234: // hack to match the FrontEnd code Can you add some more context to this .. which part of the FE code is this referring to ? http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java: http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteQueryParser.java@53 PS17, Line 53: Finished parse step, but unknown result: nit: The two part sentence could be just a single one : "Parser produced an unknown output: " http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java: http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteRelNodeConverter.java@91 PS17, Line 91: Finished RelNodeConverter step, but unknown result: nit: similar to above. http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/NotSupported.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/util/NotSupported.java: http://gerrit.cloudera.org:8080/#/c/21109/17/java/calcite-planner/src/main/java/org/apache/impala/calcite/util/NotSupported.java@26 PS17, Line 26: public static String VIEWS = "Views are not yet supported."; The not supported messages should be consistent. Some say 'not yet supported'. some say ' .. currently supported', some are omitting the 'yet'. I think best to omit the 'yet' . -- 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: 17 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Fri, 05 Apr 2024 23:09:19 +0000 Gerrit-HasComments: Yes
