Steve Carlin 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 31: (4 comments) http://gerrit.cloudera.org:8080/#/c/22319/31/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/22319/31/common/thrift/Query.thrift@778 PS31, Line 778: 192: optional bool use_calcite_planner = false; > very small nit: every other entry has a comment, even though they're pretty Done http://gerrit.cloudera.org:8080/#/c/22319/31/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/31/fe/src/main/java/org/apache/impala/service/Frontend.java@2319 PS31, Line 2319: LOG.info("Calcite planner failed: " + e); > I think this should be a separate argument, not concatenated. slf4j has spe Done http://gerrit.cloudera.org:8080/#/c/22319/31/fe/src/main/java/org/apache/impala/service/Frontend.java@3577 PS31, Line 3577: LOG.info("Using Impala Planner."); > This ends up printing "Using Impala Planner" twice, once from here and once Done http://gerrit.cloudera.org:8080/#/c/22319/31/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/22319/31/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@112 PS31, Line 112: } catch (UnsupportedFeatureException e) { > Why did you switch this to try/catch handling? That seems unnecessary and a This is actually code that's going away soon, but not immediately. The code will all eventually be run from the Frontend framework which is the purpose of this commit. So the "checkOptionSupportedInCalcite" is also called from CalciteCompilerFactory.checkParsedStatement. It makes more sense there to throw an exception because the Frontend.java handles the retry logic. Therefore, since this is temporary, I wanted to put "checkOption..." in its final form. I haven't filed a Jira yet mentioning we will get rid of this, but it is part of the natural flow of the general Calcite project. But now that you mention it, it's probably better to put the method in its final to-be resting place so that we don't have to move the code later. Open to suggestions if you think something else should be done. -- 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: 31 Gerrit-Owner: Steve Carlin <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Reviewer: Steve Carlin <[email protected]> Gerrit-Comment-Date: Sat, 29 Mar 2025 13:41:50 +0000 Gerrit-HasComments: Yes
