Vuk Ercegovac 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: (20 comments) thanks for the changes! the refactor for the tests looks great. most follow-up comments are oriented around naming. 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 supported at the beginning of the statement and before the query." 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. 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?) 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, I'd use more descriptive names ("oracle" and "default" don't convey much). How about the following: // Determines the location of optional hints. The AtStatment option // is motivated by Oracle's hint placement at the start of the // statement and the AtQuery option places the hint right before // the query (if specified). // // Examples: // Start: INSERT /* my hint */ <tablename> ... SELECT ... // End: INSERT <tablename> /* my hint */ SELECT ... public enum HintLocation { Start, End }; 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) if null? 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 the hint location). 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 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 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 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 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: Injects hints into pattern and checks that the injected hints match the expected hints. 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 and upserts. Update the comment to state that its used for both inserts and updates. 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. http://gerrit.cloudera.org:8080/#/c/8676/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@396 PS4, Line 396: TestInsertHints much better-- no redundancy added and test coverage expanded. thank you! 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: InjectInsertHint 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 http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/insert.test@460 PS4, Line 460: on nit: at 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 whether hinting works. We have one test above now that tests the two locations, so I'd omit all added tests at and after L613. 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 http://gerrit.cloudera.org:8080/#/c/8676/4/testdata/workloads/functional-planner/queries/PlannerTest/kudu-upsert.test@225 PS4, Line 225: on nit: at -- 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 <jinc...@gmail.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: John Russell <jruss...@cloudera.com> Gerrit-Reviewer: Kim Jin Chul <jinc...@gmail.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 13 Dec 2017 17:56:36 +0000 Gerrit-HasComments: Yes
