Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/8676 )
Change subject: IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT ...................................................................... Patch Set 2: (4 comments) thanks for the updates! couple of comments to round out the testing and improve the test refactor. http://gerrit.cloudera.org:8080/#/c/8676/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8676/2//COMMIT_MSG@7 PS2, Line 7: Adopt The jira says "adopt", but we're really not making this style the default. This change adds an additional hinting style to coexist with the current placement. I'd just say: Adds Oracle-style hint placement for INSERT/UPSERT I'd also add a simple example to illustrate the new placement and update the testing section as needed pending the other comments. http://gerrit.cloudera.org:8080/#/c/8676/2/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/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@44 PS2, Line 44: These tests confirm that the oracle style hints are indeed added as expected for inserts/select and upsert. Looking at https://gerrit.sjc.cloudera.com/#/c/4235/, which added additional hinting styles, we should also add the following tests: - to-sql: test/.../analysis/ToSqlTest [this patch will print the hint always in the default location. Look at L916 of main/.../analysis/InsertStmt. It should be an isolated change to modify toSql to print in the user specified location... will need a member to record the location style, an additional constructor arg, and modified copy constructor)] - end-to-end test: testdata/workloads/functional-planner/queries/PlannerTest/insert.test (look at L450) http://gerrit.cloudera.org:8080/#/c/8676/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@410 PS2, Line 410: "shuffle" This difference is preventing you from having one loop and factoring lines such as L381-383. After looking at this, I'd go with the other suggestion I had to adjust TestInsertHints to include the loop over hint placement. You can adjust the signature of TestInsertHints to be: TestInsertHints(String pattern, String hint, String... expectedHints) The pattern input can just be the string you'd pass in to String.format (so do the String.format in TestInsertHint). The hint can be what you currently have where the prefix/suffix are varied. http://gerrit.cloudera.org:8080/#/c/8676/2/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@426 PS2, Line 426: ParsesOk pls add a TestUpsertHints method that is similar to TestInsertHints so that it gets equivalent test coverage as TestInsertHints (note that ParsesOk does not verify that hints are actually attached to the statement). -- 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: 2 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: Tue, 12 Dec 2017 18:10:41 +0000 Gerrit-HasComments: Yes
