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 <anuragmantr...@gmail.com> Gerrit-Reviewer: Anurag Mantripragada <anuragmantr...@gmail.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 03 Oct 2018 07:32:57 +0000 Gerrit-HasComments: Yes
