[Impala-ASF-CR] IMPALA-12872: Use Calcite for optimization - part 1: simple queries

["Aman Sinha (Code Review)" <ge...@cloudera.org>]

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 <sc...@cloudera.com>
Gerrit-Reviewer: Aman Sinha <am...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Steve Carlin <sc...@cloudera.com>
Gerrit-Comment-Date: Fri, 05 Apr 2024 23:09:19 +0000
Gerrit-HasComments: Yes