impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Lars Volker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4167: Support insert plan hints for CREATE TABLE AS SELECT
Date Fri, 17 Nov 2017 02:51:42 GMT
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 <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, 17 Nov 2017 02:51:42 +0000
Gerrit-HasComments: Yes

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