[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join .. Patch Set 4: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/918/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 4 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 21:26:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join .. Patch Set 3: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/917/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 3 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 20:16:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11568/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11568/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1849 PS4, Line 1849: assertTrue(ctx.getAnalyzer().isStraightJoin()); > An improvement to this collection of tests would be to wrap the pattern of There are three things that need to be tested. When hint is unrecognized: 1) Appropriate warning should be added. 2) straight_join must not be set. When hint is straight_join: 3) Assert straight_join is set. I think wrapping these three things in a method is a good idea like you said. -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 4 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 17:26:01 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join
Bharath Vissapragada has posted comments on this change. (
http://gerrit.cloudera.org:8080/11568 )
Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join
..
Patch Set 4:
(4 comments)
Just a bunch of nits I noticed. Looks like there are two reviewers on this one
already. I'll let them +2 this. Change lgtm from a high level pending nits and
other reviewers' comments.
http://gerrit.cloudera.org:8080/#/c/11568/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11568/4//COMMIT_MSG@7
PS4, Line 7: Unrecognized hints are interpreted as straight_join
We typically describe the fix rather than the problem, something like
Do not interpret unrecognized hints as straight_join hints
http://gerrit.cloudera.org:8080/#/c/11568/4//COMMIT_MSG@9
PS4, Line 9: Call to setIsStraightJoin() is outside else clause in
SelectList.java
Remove. No need to describe the commit. Better to keep the commit message
high-level.
http://gerrit.cloudera.org:8080/#/c/11568/4//COMMIT_MSG@14
PS4, Line 14: Testing: Added two test cases to TestSelectListHints.
: 1) To assert straight_join is not set when hint is unrecognized
and
: 2) To assert it is properly set when hint is indeed a straigh_join
Same as above. Maybe just say something like, added unit tests for positive and
-ve test cases.
http://gerrit.cloudera.org:8080/#/c/11568/4/fe/src/main/java/org/apache/impala/analysis/SelectList.java
File fe/src/main/java/org/apache/impala/analysis/SelectList.java:
http://gerrit.cloudera.org:8080/#/c/11568/4/fe/src/main/java/org/apache/impala/analysis/SelectList.java@86
PS4, Line 86: if (!hint.is("straight_join")) {
: analyzer.addWarning("PLAN hint not recognized: " + hint);
: } else {
: analyzer.setIsStraightJoin();
: }
nit: How about inverting it? I think that is more readable.
if (hint.is("straight_join) {
setIsStraightJoin();
} else {
addWarning()
}
--
To view, visit http://gerrit.cloudera.org:8080/11568
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04
Gerrit-Change-Number: 11568
Gerrit-PatchSet: 4
Gerrit-Owner: Anurag Mantripragada
Gerrit-Reviewer: Anurag Mantripragada
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Bikramjeet Vig
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Vuk Ercegovac
Gerrit-Comment-Date: Wed, 03 Oct 2018 07:32:57 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/11568/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11568/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1849 PS4, Line 1849: assertTrue(ctx.getAnalyzer().isStraightJoin()); An improvement to this collection of tests would be to wrap the pattern of ctx; AnalyzeOk; assertTrue|False in a helper method, say expectStraightJoin(query, warning?, true| false). That way, each of these asserts that the query analyzes, checks a warning message if applicable, and states whether the hint was applied. Also, is the test on L1841 testing the same thing as the new one on L1852? -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 4 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 07:28:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join .. Patch Set 4: (5 comments) Thanks Vuk, submitted new patch. http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@7 PS3, Line 7: IMPALA-7484: Unrecognized hints are interpreted as straight_join > add a newline Done http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@8 PS3, Line 8: > wrap this appropriately. Done http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@10 PS3, Line 10: causing even unrecognized hints to be interpreted as straight_joins. > same here, please wrap the lines. Done http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1845 PS3, Line 1845: // If hint is straight join > add a space after '//' Done http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1851 PS3, Line 1851: // Unrecognized hint > same here, add a space. Done -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 4 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 07:20:12 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join
Anurag Mantripragada has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join .. IMPALA-7484: Unrecognized hints are interpreted as straight_join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted as straight_joins. Moved it into an else. Now it will be set only if the hint is a straight_join. Testing: Added two test cases to TestSelectListHints. 1) To assert straight_join is not set when hint is unrecognized and 2) To assert it is properly set when hint is indeed a straigh_join. Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 --- M fe/src/main/java/org/apache/impala/analysis/SelectList.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java 2 files changed, 13 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/11568/4 -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 4 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted
Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted as straight_joins. Moved it into an else. Now it will be called only i .. Patch Set 3: (5 comments) http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@7 PS3, Line 7: IMPALA-7484: Unrecognized hints are interpreted as straight_join add a newline http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@8 PS3, Line 8: Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted as straight_joins. Moved it into an else. Now it will be called only if the hintis a straight_join. wrap this appropriately. http://gerrit.cloudera.org:8080/#/c/11568/3//COMMIT_MSG@10 PS3, Line 10: Testing: Added two test cases to TestSelectListHints. 1) To make assert straight_join is not set when hint is unrecognized and 2) To assert it is properly set when hint is indeed a straigh_join. same here, please wrap the lines. http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1845 PS3, Line 1845: //if hint is straight join add a space after '//' http://gerrit.cloudera.org:8080/#/c/11568/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1851 PS3, Line 1851: //Unrecognized hint same here, add a space. -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 3 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 03 Oct 2018 05:50:33 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted
Anurag Mantripragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted as straight_joins. Moved it into an else. Now it will be called only i .. Patch Set 3: (5 comments) Thanks Fredy for the inputs. Uploaded new patch set. Please take a look. http://gerrit.cloudera.org:8080/#/c/11568/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11568/2//COMMIT_MSG@7 PS2, Line 7: IMPALA-7484: Unrecognized hints are interpreted as straight_join > Our convention for the commit message is: Done http://gerrit.cloudera.org:8080/#/c/11568/2/fe/src/main/java/org/apache/impala/analysis/SelectList.java File fe/src/main/java/org/apache/impala/analysis/SelectList.java: http://gerrit.cloudera.org:8080/#/c/11568/2/fe/src/main/java/org/apache/impala/analysis/SelectList.java@89 PS2, Line 89: analyzer.setIsStraightJoin(); > nit: move L89 to L88 Done http://gerrit.cloudera.org:8080/#/c/11568/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/11568/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1844 PS2, Line 1844: ctx = createAnaly > nit: rename to ctx Done http://gerrit.cloudera.org:8080/#/c/11568/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1847 PS2, Line 1847: "select %sstraight_join%s * from functional.alltypes", prefix, suffix), : ctx); > nit: fix indentation Done http://gerrit.cloudera.org:8080/#/c/11568/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@1849 PS2, Line 1849: ssertTrue(ctx.getAnalyzer().isStraightJoin()); > We also need another test case where assertFalse(ctx.getAnalyzer().isStraig Done -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 3 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Comment-Date: Wed, 03 Oct 2018 04:10:16 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7484: Unrecognized hints are interpreted as straight join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted
Anurag Mantripragada has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11568 ) Change subject: IMPALA-7484: Unrecognized hints are interpreted as straight_join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted as straight_joins. Moved it into an else. Now it will be called only i .. IMPALA-7484: Unrecognized hints are interpreted as straight_join Call to setIsStraightJoin() is outside else clause in SelectList.java causing even unrecognized hints to be interpreted as straight_joins. Moved it into an else. Now it will be called only if the hintis a straight_join. Testing: Added two test cases to TestSelectListHints. 1) To make assert straight_join is not set when hint is unrecognized and 2) To assert it is properly set when hint is indeed a straigh_join. Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 --- M fe/src/main/java/org/apache/impala/analysis/SelectList.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java 2 files changed, 13 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/68/11568/3 -- To view, visit http://gerrit.cloudera.org:8080/11568 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Icf600ebbfefc7398e0896df143a0ab91545cae04 Gerrit-Change-Number: 11568 Gerrit-PatchSet: 3 Gerrit-Owner: Anurag Mantripragada Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins
