Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22412 )
Change subject: IMPALA-13571: Calcite Planner: Fix join parsing errors. ...................................................................... Patch Set 4: (5 comments) http://gerrit.cloudera.org:8080/#/c/22412/4/java/calcite-planner/src/main/codegen/templates/Parser.jj File java/calcite-planner/src/main/codegen/templates/Parser.jj: http://gerrit.cloudera.org:8080/#/c/22412/4/java/calcite-planner/src/main/codegen/templates/Parser.jj@1307 PS4, Line 1307: ( <STRAIGHT_JOIN> )? There are 3 variations: SELECT straight_join a1, a2 FROM .. SELECT /* +straight_join */ a1, a2 FROM .. SELECT [straight_join] a1, a2 FROM Also, could you pls add the CLUSTERED hint since it is part of the same SELECT statement ? Heres the correspond hint in the Impala Cup parser: https://github.com/apache/impala/blob/master/fe/src/main/cup/sql-parser.cup#L3576 http://gerrit.cloudera.org:8080/#/c/22412/4/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/22412/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteJniFrontend.java@224 PS4, Line 224: if (LEFT_ANTI.matcher(s).matches()) { nit: you could combine the checks for left, right anti joins since the error message is the same. Similarly for left, right semi join. http://gerrit.cloudera.org:8080/#/c/22412/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java File java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java: http://gerrit.cloudera.org:8080/#/c/22412/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@127 PS4, Line 127: * Populate the CalciteSchema with tables being used by this query. The returned list of table names is actually the error tables, not the valid ones. It would be best to update the comment here to indicate that. A suggestion for the terminology: the term 'errorTables' is getting overloaded throughout the patch. It seems you want 2 lists: a list of not found tables (these don't exist in the catalog), and a list of tables with the complex column. I think it would be useful to track these separately. http://gerrit.cloudera.org:8080/#/c/22412/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@211 PS4, Line 211: // TODO: 'complex' tables ignored for now I suppose this TODO can be removed in this patch since there's an error message generated. http://gerrit.cloudera.org:8080/#/c/22412/4/java/calcite-planner/src/main/java/org/apache/impala/calcite/service/CalciteMetadataHandler.java@277 PS4, Line 277: StmtMetadataLoader errorLoader = new StmtMetadataLoader( Shouldn't this StmtMetadataLoader be instantiated only once for this class ? We don't need to create it each time this method is called (even though currently this is called once). This errorLoader does not depend on any local variables. -- To view, visit http://gerrit.cloudera.org:8080/22412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icd0f68441c84b090ed2cb45de96ccee1054deef5 Gerrit-Change-Number: 22412 Gerrit-PatchSet: 4 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-Comment-Date: Mon, 03 Feb 2025 07:34:36 +0000 Gerrit-HasComments: Yes
