Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/9933 )
Change subject: IMPALA-2195: Improper handling of comments in queries ...................................................................... Patch Set 9: (10 comments) http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@320 PS7, Line 320: sys.exit(1) > nit: Add a comment that we are reconstructing the query stmt by adding the Done http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@322 PS7, Line 322: comment (if exists) will be > Can be simplified with -- self.comment or "" Done http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@590 PS7, Line 590: # is necessary to find a proper function and here is a right place > can we call it prefix_comments/leading_comments to make it more clearer? co Done http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1288 PS7, Line 1288: self._disable_readline() > I think we need to expand the method doc a little more. Why we strip the le Done http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1289 PS7, Line 1289: > , comment Done http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1294 PS7, Line 1294: ten original line, with leadi > nit:reword to ..deduce the 'command' part ... maybe? Done http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1295 PS7, Line 1295: and trailing space removed and special characters transfor > nit: Already in the beginning, keep one. Done http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1306 PS7, Line 1306: args: connect > nit: Returns (comment, query.....? Done. docstring updated. IMO it's better to keep it here and keep only bug fixes in shell/ext-py/sqlparse . It makes it easier to upgrade sqlparse in the future. http://gerrit.cloudera.org:8080/#/c/9933/7/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/9933/7/tests/shell/test_shell_interactive.py@545 PS7, Line 545: def test_strip_leading_comment(self): > Add a method doc. Done http://gerrit.cloudera.org:8080/#/c/9933/7/tests/shell/test_shell_interactive.py@565 PS7, Line 565: ImpalaShellClass.strip_leading_comment('select /*do not delete*/ 1') > Can you add one example with join hints too? SHUFFLE etc. Done -- To view, visit http://gerrit.cloudera.org:8080/9933 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7ac7cb5a30e6dda73ebe761d9f0eb9ba038e14a7 Gerrit-Change-Number: 9933 Gerrit-PatchSet: 9 Gerrit-Owner: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Comment-Date: Mon, 30 Apr 2018 21:45:33 +0000 Gerrit-HasComments: Yes
