Lars Volker 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 4: (7 comments) 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@1167 PS3, Line 1167: tbl_def_without_col_defs ::= > It is also accepted in that case, but has no effect - should I disallow it? It's important that it is not accepted: we should only accept supported options and should issue warnings for unsupported ones. It seems ok to me to decide on which hints are allows in the Java code, unless there is an easy way to do that in the Parser. 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 > This was copy pasted - I guess it means "not error, just warning". Did you find a good way? If not, please clean up this code with its own intent in mind. I think that will result in cleaner code than keeping it around with good intentions. Currently any such plans are undocumented and the next person to look at it will have a harder time understanding it. If we want to go back there's always git. :) 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. > This is also copy paste legacy - AnalyzeStmtsTest.TestInsertHints() had an I see. I think it should go somewhere near the top. See my previous comment, too. http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1653 PS3, Line 1653: > Also copy paste legacy. See above. http://gerrit.cloudera.org:8080/#/c/8400/4/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test File testdata/workloads/functional-planner/queries/PlannerTest/ddl.test: http://gerrit.cloudera.org:8080/#/c/8400/4/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@207 PS4, Line 207: noclustered nit: remove the "" or add them in the test below? http://gerrit.cloudera.org:8080/#/c/8400/4/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@233 PS4, Line 233: +noclustered,shuffle nit: spaces http://gerrit.cloudera.org:8080/#/c/8400/4/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@251 PS4, Line 251: # IMPALA-4167: noclustered hint in CTAS into partitioned HDFS table has no effect as it I think it's better to not mention the default behavior here. The important thing to test is that specifying "noclustered" results in a plan without an additional sort node. At some point we may want to switch the default and this test should make sure we don't break noclustered. -- 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: 4 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Comment-Date: Fri, 17 Nov 2017 02:51:42 +0000 Gerrit-HasComments: Yes
