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 <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: Tue, 12 Dec 2017 01:50:13 +0000 Gerrit-HasComments: Yes
