Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/8400 )
Change subject: IMPALA-4167: Support insert plan hints for CREATE TABLE AS SELECT ...................................................................... Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/8400/11/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/8400/11/fe/src/main/cup/sql-parser.cup@1065 PS11, Line 1065: create_tbl_as_select_stmt_inner ::= > let's call this create_tbl_as_select_params to match what is produced Done http://gerrit.cloudera.org:8080/#/c/8400/6/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: http://gerrit.cloudera.org:8080/#/c/8400/6/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@69 PS6, Line 69: * Helper class for parsing. > I looked at this closely and both approaches seem fine to me. Here are my t Thanks! http://gerrit.cloudera.org:8080/#/c/8400/11/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: http://gerrit.cloudera.org:8080/#/c/8400/11/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@76 PS11, Line 76: public CreateTableStmt createStmt_; > We don't use the "_" for public members. Done http://gerrit.cloudera.org:8080/#/c/8400/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/8400/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1642 PS11, Line 1642: private String[][] getHintStyles() { > Can you move this to FrontendTestBase, make it protected, and use it in the I have moved the member variable version found in other files instead of the function - I prefer the member, because no new arrays have to be created on every call. http://gerrit.cloudera.org:8080/#/c/8400/11/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1651 PS11, Line 1651: public void TestCreateTableAsSelectWithHints() throws AnalysisException { > * I think the more important testing is in ParserTest#TestPlanHints similar I have added the CTAS version of insert tests to ParserTest#TestPlanHints. I think that some of these tests should move there, like the ones about the case insensitivity of hints, while the ones with errors or warnings should remain here. I am unsure about the rest, maybe they can be deleted, as planner tests already check most of these cases. About combining with TestInsertHints(): I have created combined insert/upsert/ctas tests in ParserTest#TestPlanHints, something similar could be done here. What makes this more tricky is that the actual tables are checked, so the source table must exist, and the target table in insert or the partitioning parameters in CTAS must match with the result of the select + there are some cases where insert is possible, but CTAS is not (HBase tables). http://gerrit.cloudera.org:8080/#/c/8400/11/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test File testdata/workloads/functional-planner/queries/PlannerTest/ddl.test: http://gerrit.cloudera.org:8080/#/c/8400/11/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@207 PS11, Line 207: # IMPALA-4167: noclustered hint is ignored if the CTAS has partitions and sort columns. > * Any ideas to trim down the tests? Seems like a lot of new tests and we sh Maybe create /*+ clustered,noshuffle */ + the default case without hints is enough to show that both type of hints can take effect. If the other tests are deleted, then some of them should be moved to the insert tests, because they cover some scenarios that are not tested there. What do you think about this solution? I agree that both plans are unnecessary - I kept the DISTRIBUTEDPLAN, because the effect of (no)shuffle can be only checked there. -- To view, visit http://gerrit.cloudera.org:8080/8400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8d74bca999da8ae1bb89427c70841f33e3c56ab0 Gerrit-Change-Number: 8400 Gerrit-PatchSet: 11 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Comment-Date: Wed, 07 Feb 2018 17:50:38 +0000 Gerrit-HasComments: Yes
