Alex Behm 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 14: (6 comments) http://gerrit.cloudera.org:8080/#/c/8400/14/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java File fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java: http://gerrit.cloudera.org:8080/#/c/8400/14/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@82 PS14, Line 82: Preconditions.checkNotNull(queryStmt); combine with member assignment: this.createStmt = Preconditions.checkNotNull(queryStmt); http://gerrit.cloudera.org:8080/#/c/8400/14/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/14/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1640 PS14, Line 1640: public void TestCreateTableAsSelectWithHints() throws AnalysisException { To avoid duplication with AnalyzeStmts#TestInsertHints let's do the following: * Keep a single basic test, but remove all other tests that are basically the same as in AnalyzeStmts#TestInsertHints * Keep those tests that are different from the expected behavior in AnalyzeStmts#TestInsertHints * Add a comment that ParserTest ensures hints are properly set in the InsertStmt of a Ctas, and that AnalyzeStmts#TestInsertHints covers the analysis of those hints. Therefore, the tests here are minimal. http://gerrit.cloudera.org:8080/#/c/8400/14/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/14/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@304 PS14, Line 304: * Creates an insert, an upsert, and a CTAS statement with the given hints Upsert isn't tested. Code or comment wrong? http://gerrit.cloudera.org:8080/#/c/8400/14/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@336 PS14, Line 336: static private List<String> ConvertHintsToStringList(List<PlanHint> hints) { shorter: hintsToStrings() Easier to return them as a String[], then you don't need to convert expectedHints to an List in L323 http://gerrit.cloudera.org:8080/#/c/8400/14/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java: http://gerrit.cloudera.org:8080/#/c/8400/14/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@83 PS14, Line 83: new String[] { "/* +", "*/" }, // traditional commented hint tabs http://gerrit.cloudera.org:8080/#/c/8400/14/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test File testdata/workloads/functional-planner/queries/PlannerTest/ddl.test: http://gerrit.cloudera.org:8080/#/c/8400/14/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@207 PS14, Line 207: # IMPALA-4167: noclustered hint is ignored if the CTAS has partitions and sort columns. Let's follow a procedure to trim down these tests similar to what we are doing for AnalyzeDDLTest, i.e.: * Keep 1-2 basic tests. * Comment the hints only affect the insert portion which is already tested more thoroughly elsewhere (add missing overage to the insert tests as necessary if you think coverage is missing) * Keep tests that are truly specific to Ctas (though I think there are none) -- 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: 14 Gerrit-Owner: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Comment-Date: Sat, 17 Feb 2018 17:58:55 +0000 Gerrit-HasComments: Yes
