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 3: (13 comments) http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG@9 PS3, Line 9: Adding support for "clustered", "noclustered", "shuffle" and "noshuffle" > How about "This change adds support..." Done http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG@12 PS3, Line 12: example: > Capitalize Done http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG@19 PS3, Line 19: Sort by partition columns before insert to make the insert more efficient. > Mention where exactly the sort happens (locally vs distributed). Done http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG@26 PS3, Line 26: exchenge > spelling Done http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/cup/sql-parser.cup@1054 PS3, Line 1054: RESULT = new CreateTableAsSelectStmt(new CreateTableStmt(tbl_def), > nit: Can you wrap the lines at 90 chars? Done http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/cup/sql-parser.cup@1167 PS3, Line 1167: tbl_def_without_col_defs ::= > Does that now also allow hints like "CREATE /*+ clustered */ TABLE FOO LIKE It is also accepted in that case, but has no effect - should I disallow it? I think that it would be the best to enable hints in several cases (even if no hint is accepted there currently), and check the hints in normal java code and warn if one is not valid for a given statement. I am not sure though. http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/cup/sql-parser.cup@1168 PS3, Line 1168: KW_CREATE external_val:external opt_plan_hints:hints KW_TABLE if_not_exists_val:if_not_exists > long line Done http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java File fe/src/main/java/org/apache/impala/analysis/TableDef.java: http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@155 PS3, Line 155: TableDef(TableName tableName, boolean isExternal, boolean ifNotExists, List<PlanHint> hints) { > long line Done http://gerrit.cloudera.org:8080/#/c/8400/3/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/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1633 PS3, Line 1633: Only > What does "only" mean here? This was copy pasted - I guess it means "not error, just warning". I did not want to deviate too much from the original (AnalyzeStmtsTest.TestInsertHints()), to make it easier to merge the two if I find a good way to do it. http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1647 PS3, Line 1647: // HBase tables cannot be tested, as currently Impala cannot create HBase tables. > It's weird that this explanation occurs in the middle of the function. Can This is also copy paste legacy - AnalyzeStmtsTest.TestInsertHints() had an HBase test here. http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1653 PS3, Line 1653: > Why is there a newline? Also copy paste legacy. http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1677 PS3, Line 1677: "select * from functional.alltypes"); > Please also add tests that have additional hints in the select clause. Done http://gerrit.cloudera.org:8080/#/c/8400/3/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test File testdata/workloads/functional-planner/queries/PlannerTest/ddl.test: http://gerrit.cloudera.org:8080/#/c/8400/3/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@207 PS3, Line 207: # IMPALA-4167: clustered CTAS into partitioned table adds sort node. > Can you add tests for noclustered and for sort by()? Done -- 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: 3 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Comment-Date: Fri, 17 Nov 2017 00:09:02 +0000 Gerrit-HasComments: Yes
