impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Csaba Ringhofer (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4167: Support insert plan hints for CREATE TABLE AS SELECT
Date Fri, 22 Dec 2017 15:14:05 GMT
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 <csringhofer@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <csringhofer@cloudera.com>
Gerrit-Reviewer: Gabor Kaszab <gaborkaszab@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Comment-Date: Fri, 22 Dec 2017 15:14:05 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message