Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22319 )
Change subject: IMPALA-13657: Connect Calcite planner to Impala Frontend framework ...................................................................... Patch Set 34: (9 comments) me http://gerrit.cloudera.org:8080/#/c/22319/33/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/22319/33/common/thrift/Query.thrift@778 PS33, Line 778: // See comment in ImpalaService.thrift : 192: optional bool use_calcite_planner = false; > Please also add appropriate test in query-options-test.cc. The list of planners along with an implied precedence order makes sense, although I am ok if that is done in a later enhancement patch. Agree with adding tests to query-options-test http://gerrit.cloudera.org:8080/#/c/22319/34/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/22319/34/fe/src/main/java/org/apache/impala/service/Frontend.java@2324 PS34, Line 2324: Impala nit: see related comment http://gerrit.cloudera.org:8080/#/c/22319/34/fe/src/main/java/org/apache/impala/service/Frontend.java@2325 PS34, Line 2325: legacy nit: see related comment http://gerrit.cloudera.org:8080/#/c/22319/34/fe/src/main/java/org/apache/impala/service/Frontend.java@2336 PS34, Line 2336: legacy nit: see above http://gerrit.cloudera.org:8080/#/c/22319/34/fe/src/main/java/org/apache/impala/service/Frontend.java@3571 PS34, Line 3571: regular nit: In the previous patch (IMPALA-13653) that was merged, you had used "original planner" in the CompilerFactory. Let's use that same consistent terminology here and other places. http://gerrit.cloudera.org:8080/#/c/22319/34/fe/src/main/java/org/apache/impala/service/Frontend.java@3574 PS34, Line 3574: regular nit: see above http://gerrit.cloudera.org:8080/#/c/22319/34/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteCompilerFactory.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteCompilerFactory.java: http://gerrit.cloudera.org:8080/#/c/22319/34/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteCompilerFactory.java@24 PS34, Line 24: import org.apache.impala.analysis.AnalysisContext.AnalysisDriverImpl; nit: several of these imports don't seem to be used in this class. You can check in the IDE and remove the unused ones. http://gerrit.cloudera.org:8080/#/c/22319/34/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteCompilerFactory.java@101 PS34, Line 101: Decimal v2 This says v2 but I thought v2 is supported via Calcite, v1 is not. IMPALA-13530 also says v1. http://gerrit.cloudera.org:8080/#/c/22319/33/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteSingleNodePlanner.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteSingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/22319/33/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteSingleNodePlanner.java@81 PS33, Line 81: rootNodeSmap nit: this input parameter is not used here.. can you add a comment for the method ? It is needed for the original planner but not for Calcite. -- To view, visit http://gerrit.cloudera.org:8080/22319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3b30571beb797ede827ef4d794b8daefb130ccb1 Gerrit-Change-Number: 22319 Gerrit-PatchSet: 34 Gerrit-Owner: Steve Carlin <scar...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Anonymous Coward (816) Gerrit-Reviewer: Fang-Yu Rao <fangyu....@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: Noemi Pap-Takacs <npaptak...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Steve Carlin <scar...@cloudera.com> Gerrit-Comment-Date: Tue, 01 Apr 2025 16:15:22 +0000 Gerrit-HasComments: Yes
