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 17:

(3 comments)

I have marked the lines where the rebase (patch set 16) led to non-trivial 
conflicts/broken tests. Patch set 17 contains some typo fixes + changes in 
kudu.test, which I forgot to run after rebase.

http://gerrit.cloudera.org:8080/#/c/8400/17/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

http://gerrit.cloudera.org:8080/#/c/8400/17/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@303
PS17, Line 303:   /**
              :    * Creates an insert into, an insert overwrite, and a CTAS 
statement with
              :    * the given hints and checks that the parsed hints are the 
same as the expected hints.
              :    */
              :   private void TestInsertAndCtasHints(String insertPart, String 
ctasPart,
              :         String[] hintStyle, String hints, String... 
expectedHints) {
              :     String hintsPart = hintStyle[0] + hints + hintStyle[1];
              :     TestInsertStmtHints(String.format("insert %%s into %s %%s 
select * from t",
              :         insertPart), hintsPart, expectedHints);
              :     TestInsertStmtHints(String.format("insert %%s overwrite %s 
%%s select * from t",
              :         insertPart), hintsPart, expectedHints);
              :     TestCtasHints(String.format("create %s table %s as select * 
from t",
              :         hintsPart, ctasPart), expectedHints);
              :   }
              :
              :   /**
              :    * Injects hints into pattern and checks that the injected 
hints match the expected
              :    * hints. This function covers both insert and upsert 
statements.
              :    */
              :   private void TestInsertStmtHints(String pattern, String hint, 
String... expectedHints) {
              :     for (InsertStmt.HintLocation loc: 
InsertStmt.HintLocation.values()) {
              :       InsertStmt insertStmt = (InsertStmt) 
ParsesOk(InjectInsertHint(pattern, hint, loc));
              :       assertEquals(expectedHints, 
HintsToStrings(insertStmt.getPlanHints()));
              :     }
              :   }
              :
              :   /**
              :    * Injects hints into pattern and expect parser error on the 
injected hints.
              :    * It covers both insert and upsert statements.
              :    */
              :   private void ParserErrorOnInsertStmtHints(String pattern, 
String hint) {
              :     for (InsertStmt.HintLocation loc: 
InsertStmt.HintLocation.values()) {
              :       ParserError(InjectInsertHint(pattern, hint, loc));
              :     }
              :   }
IMPALA-4168: "Adds Oracle-style hint placement for INSERT/UPSERT" also contains 
similar changes to avoid test duplication ( 
https://gerrit.cloudera.org/#/c/8676/ ).


http://gerrit.cloudera.org:8080/#/c/8400/17/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test
File testdata/workloads/functional-planner/queries/PlannerTest/ddl.test:

http://gerrit.cloudera.org:8080/#/c/8400/17/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@210
PS17, Line 210: # noshuffle hint would lead to a different plan.
              : # Note that noclustered hint is added to ensure consistent 
plans on Impala 2.x and 3.x,
              : # because IMPALA-5293 changed clustered to be the default on 
3.x.
              : create /*+ noclustered */table t partitioned by (year, month) as
These tests were broken by IMPALA-5293: "Turn insert clustering on by default". 
IMPALA-5293 is not merged to Impala 2.x, while this change can be merged in my 
opinion. To avoid extra work, I have changed the tests to statements that 
should lead to the same plan on both branches.


http://gerrit.cloudera.org:8080/#/c/8400/17/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test
File testdata/workloads/functional-planner/queries/PlannerTest/kudu.test:

http://gerrit.cloudera.org:8080/#/c/8400/17/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test@431
PS17, Line 431: primary key(id) partition by hash(id) partitions 3 stored as 
kudu as
              : select * from functional.alltypes;
              : ---- DISTRIBUTEDPLAN
              : INSERT INTO KUDU [default.t]
              : |
              : 01:EXCHANGE [KUDU(KuduPartition(functional.alltypes.id))]
              : |
              : 00:SCAN HDFS [functional.alltypes]
              :    partitions=24/24 files=24 size=478.45KB
              : ====
              : create /* +noclustered,noshuffle */ table t
              : primary key(id) partition by hash(id) partitions 3 stored as 
kudu as
These tests broke because of IMPALA-6297: "Don't partition/sort for DML on 
unpartitioned Kudu table",



--
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: 17
Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@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: Thu, 22 Feb 2018 15:11:33 +0000
Gerrit-HasComments: Yes

Reply via email to