Alex Behm 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 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 don't feel strongly either, lets see what others think. I looked at this closely and both approaches seem fine to me. Here are my thoughts: * We follow the set() approach in a few places because it's more practical than the alternative * Relying on a post-construction set() can be harder to reason about, so I generally prefer the current approach where all info is available at construction time I suggest we proceed with the current version. Nice work on navigating the parser changes, Csaba! 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. 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 other appropriate places. I found this thing in: AnalyzeDDLTest.java AnalyzeStmtsTest.java ParserTest.java ToSqlTest.java 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 to what we do for INSERT. We should check that the generated InsertStmt has the hints properly set. * These tests are almost identical to AnalyzeStmts#TestInsertHints. The copy+paste worries me. * Combining this test with TestInsertHints() seems tricky, but might be doable. * Alternatively, we could shrink this test to the bare minimum and comment that coverage is provided by ParserTest#TestPlanHints (tests that hints are set in the CTAS' InsertStmt) and AnalyzeStmts#TestInsertHints() (tests analyzing an InsertStmt with hints) What do you think? 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 should have existing tests for INSERT that cover most parts. * Do we really need both PLAN and DISTRIBUTEDPLAN for these tests? -- 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, 31 Jan 2018 01:38:47 +0000 Gerrit-HasComments: Yes
