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 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's important that it is not accepted: we should only accept supported opt Done in cup. I did not find another way (that does not add a lot of duplication) than moving KW_CREATE to high level rules. The problem was that the parser cannot differentiate between the CTAS and CREATE until it reaches the AS SELECT part, but it needs this information to decide whether to add an opt_plan_hints non-terminal or not if there is no hint after CREATE. 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 > Did you find a good way? If not, please clean up this code with its own int No, I did not find a really good way. The duplication could be merged, but the resulting code would be quite hard to understand, mainly because the format + partitions of a table are given in inserts, but have to specified in CTAS. This could be done by mapping table names (used in insert) to sql options (used in CTAS), and substituting these strings in the statements, but this would ruin the readability of the statements. 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. > I see. I think it should go somewhere near the top. See my previous comment Done http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1653 PS3, Line 1653: > See above. Done 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? Done http://gerrit.cloudera.org:8080/#/c/8400/4/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@233 PS4, Line 233: +noclustered,shuffle > nit: spaces Done 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 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: 4 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, 22 Dec 2017 15:14:05 +0000 Gerrit-HasComments: Yes
