Kim Jin Chul has posted comments on this change. ( http://gerrit.cloudera.org:8080/8676 )
Change subject: IMPALA-4168: Adds Oracle-style hint placement for INSERT/UPSERT ...................................................................... Patch Set 4: (19 comments) Thanks a lot for the kind review. I can feel the improvement of code quality! http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup File fe/src/main/cup/sql-parser.cup: http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup@717 PS4, Line 717: Please beware of that the both of Oracle and default hint styles are supported when : // extending INSERT/UPSERT syntax. > Replace with: "Note: when extending INSERT/UPSERT syntax, hinting is suppor Thanks, it's more clear. http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/cup/sql-parser.cup@807 PS4, Line 807: Please beware of that the both of Oracle and default hint styles are supported when : // extending INSERT/UPSERT syntax. > make this consistent with the proposal on L717. Done http://gerrit.cloudera.org:8080/#/c/8676/4/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/8676/4/fe/src/main/java/org/apache/impala/analysis/CreateTableAsSelectStmt.java@86 PS4, Line 86: InsertStmt.HintLocation.DefaultLoc > more intuitive to pass in null here (what does DefaultLoc for no hints mean You're right. null is looks more intuitive. http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java File fe/src/main/java/org/apache/impala/analysis/InsertStmt.java: http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@56 PS4, Line 56: // A flag to determine the location of a hint > This is a good place to describe the alternatives, including examples. Also The values of the enumeration are more readable and intuitive. Thanks. http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@210 PS4, Line 210: hintLoc_ = hintLoc; > can a caller pass in null? how about we pick the default (current placement Yes, I agree on your idea. http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/main/java/org/apache/impala/analysis/InsertStmt.java@908 PS4, Line 908: hintLoc_ == HintLocation.OracleLoc && !planHints_.isEmpty() > I would flip these tests around (test for whether we have hints, then test Done http://gerrit.cloudera.org:8080/#/c/8676/4/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/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@303 PS4, Line 303: BuildQueryStmt > more specific: rename to InjectInsertHint Done http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@310 PS4, Line 310: hints > rename to actualHints Done http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@310 PS4, Line 310: TestHints > rename to VerifyHints Done http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@311 PS4, Line 311: actualHints > rename to actualStringHints Done http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@318 PS4, Line 318: Parses stmt and checks that the insert hints stmt are the expected hints. > How about: Thanks for the suggestion. It's more clear! http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@320 PS4, Line 320: TestInsertHints > rename this to: TestInsertStmtHints... our InsertStmt covers both inserts a Done http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@327 PS4, Line 327: /** : * Parses stmt and checks that the upsert hints stmt are the expected hints. : */ : private void TestUpsertHints(String pattern, String hint, String... expectedHints) { : TestInsertHints(pattern, hint, expectedHints); : } > remove-- not needed. Done http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@505 PS4, Line 505: BuildQueryStmt > for consistency with the proposed change in the parse test: InjectInsertHin Done http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/insert.test File testdata/workloads/functional-planner/queries/PlannerTest/insert.test: http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/insert.test@450 PS4, Line 450: on > nit: at Done http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/insert.test@460 PS4, Line 460: on > nit: at Done http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/insert.test@613 PS4, Line 613: same as above but the hint is on Oracle hint locat > All the tests below are tests added for bug fixes, not to explicitly test w Done http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test File testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test: http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test@204 PS4, Line 204: on > nit: at Done http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test@225 PS4, Line 225: on > nit: at Done -- To view, visit http://gerrit.cloudera.org:8080/8676 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20 Gerrit-Change-Number: 8676 Gerrit-PatchSet: 4 Gerrit-Owner: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: John Russell <[email protected]> Gerrit-Reviewer: Kim Jin Chul <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 13 Dec 2017 23:40:49 +0000 Gerrit-HasComments: Yes
