[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
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 Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: John Russell Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 12 Dec 2017 18:10:41 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
Kim Jin Chul 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: (7 comments) http://gerrit.cloudera.org:8080/#/c/8676/1/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/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@376 PS1, Line 376: "join %sshuffle%s functional.alltypes e using(string_col)", > instead of repeating the queries, how about we vary the hint location. I'm Done. Thanks for your kind example. http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@392 PS1, Line 392: estInsertHint > include jira issue for bug fixes, not new features. Thanks for the information. I didn't know about that. http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@392 PS1, Line 392: s(Str > nit: remove the "Adopt". Done http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@409 PS1, Line 409: o > we can remove redundancy here as well. Done http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@414 PS1, Line 414: inal String o > same here Done http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@483 PS1, Line 483: > and remove redundancy here as well. Done http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@499 PS1, Line 499: > same here 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: 2 Gerrit-Owner: Kim Jin Chul Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: John Russell Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Tue, 12 Dec 2017 10:59:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
Hello John Russell, Alex Behm, Vuk Ercegovac, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/8676 to look at the new patch set (#2). Change subject: IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT .. IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT testing: Add unit tests to ParserTest#TestPlanHints Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20 --- M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/ParserTest.java 2 files changed, 129 insertions(+), 34 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8676/2 -- 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: newpatchset Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20 Gerrit-Change-Number: 8676 Gerrit-PatchSet: 2 Gerrit-Owner: Kim Jin Chul Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: John Russell Gerrit-Reviewer: Kim Jin Chul Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
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 1:
(7 comments)
Thanks for the change!
I have some initial feedback for the tests.
http://gerrit.cloudera.org:8080/#/c/8676/1/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/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@376
PS1, Line 376: // Test insert hints.
instead of repeating the queries, how about we vary the hint location. I'm
thinking of something like:
for (placement : {OracleStyle, DefaultStyle}) {
hint = String.format("%sshuffle%s", prefix, suffix)
defaultHint = hint;
oracleHint = ""
if (placement == oracleStyle) {
defaultHint = "";
oracleHint = hint;
}
TestInsertHints(String.format("insert %s into t %s select * from t",
oracleHint, defaultHint),...)
TestInsertHints(...)
...
}
Another place to inject this variation may be in TestInsertHints (I see that
the upsert just checks for parsing, not whether the hints were applied as
expected-- would be useful to upgrade these while here).
http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@392
PS1, Line 392: Adopt
nit: remove the "Adopt".
http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@392
PS1, Line 392: IMPALA-4168:
include jira issue for bug fixes, not new features.
http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@409
PS1, Line 409: .
we can remove redundancy here as well.
http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@414
PS1, Line 414: IMPALA-4168:
same here
http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@483
PS1, Line 483: .
and remove redundancy here as well.
http://gerrit.cloudera.org:8080/#/c/8676/1/fe/src/test/java/org/apache/impala/analysis/ParserTest.java@499
PS1, Line 499: IMPALA-4168:
same here
--
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: 1
Gerrit-Owner: Kim Jin Chul
Gerrit-Reviewer: Alex Behm
Gerrit-Reviewer: John Russell
Gerrit-Reviewer: Kim Jin Chul
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Tue, 12 Dec 2017 01:50:13 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
Kim Jin Chul 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 1: Thanks for the information. Let me deliver documentation on a separate change. -- 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: 1 Gerrit-Owner: Kim Jin Chul Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: John Russell Gerrit-Reviewer: Kim Jin Chul Gerrit-Comment-Date: Mon, 04 Dec 2017 07:53:04 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
John Russell 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 1: Sure, if you would like to try editing the doc files. Please do that as a separate gerrit, and add me as a reviewer. -- 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: 1 Gerrit-Owner: Kim Jin Chul Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: John Russell Gerrit-Reviewer: Kim Jin Chul Gerrit-Comment-Date: Mon, 04 Dec 2017 07:50:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
Kim Jin Chul 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 1: @John, as far as I know you're in charge of the documentation. Would you please answer my question? -- 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: 1 Gerrit-Owner: Kim Jin Chul Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: John Russell Gerrit-Reviewer: Kim Jin Chul Gerrit-Comment-Date: Mon, 04 Dec 2017 07:45:52 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
Kim Jin Chul 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 1: May I add the new syntax to docs? -- 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: 1 Gerrit-Owner: Kim Jin Chul Gerrit-Reviewer: Alex Behm Gerrit-Reviewer: Kim Jin Chul Gerrit-Comment-Date: Mon, 04 Dec 2017 01:59:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT
Kim Jin Chul has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8676 Change subject: IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT .. IMPALA-4168: Adopt Oracle-style hint placement for INSERT/UPSERT testing: Add unit tests to ParserTest#TestPlanHints Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20 --- M fe/src/main/cup/sql-parser.cup M fe/src/test/java/org/apache/impala/analysis/ParserTest.java 2 files changed, 68 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/76/8676/1 -- 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: newchange Gerrit-Change-Id: Ied7629d70197a0270cdc0853e00cc021fdb4dc20 Gerrit-Change-Number: 8676 Gerrit-PatchSet: 1 Gerrit-Owner: Kim Jin Chul
