Michael Brown has posted comments on this change. ( http://gerrit.cloudera.org:8080/9933 )
Change subject: IMPALA-2195: Improper handling of comments in queries ...................................................................... Patch Set 13: (8 comments) I checked out your patch and ran this from the root Impala.git directory: git diff HEAD^ | impala-flake8 --diff I marked flake8's output for you inline. I also have some ideas for test cases (see below). http://gerrit.cloudera.org:8080/#/c/9933/13/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9933/13/shell/impala_shell.py@325 PS13, Line 325: command, args), ./shell/impala_shell.py:325:50: E128 continuation line under-indented for visual indent http://gerrit.cloudera.org:8080/#/c/9933/13/shell/impala_shell.py@1344 PS13, Line 1344: if isinstance(token, sqlparse.sql.Comment): return True ./shell/impala_shell.py:1344:51: E701 multiple statements on one line (colon) http://gerrit.cloudera.org:8080/#/c/9933/13/shell/impala_shell.py@1346 PS13, Line 1346: if token.ttype == comment: return True ./shell/impala_shell.py:1346:36: E701 multiple statements on one line (colon) http://gerrit.cloudera.org:8080/#/c/9933/13/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/9933/13/tests/shell/test_shell_interactive.py@481 PS13, Line 481: 'select * from leading_comment;') What about a test case like: /* comment1 comment2 */ select * from leading_comment; http://gerrit.cloudera.org:8080/#/c/9933/13/tests/shell/test_shell_interactive.py@482 PS13, Line 482: assert 'Fetched 1 row(s)' in result.stderr What about a test case like: /* select * from leading_comment */ select * from leading_comment; http://gerrit.cloudera.org:8080/#/c/9933/13/tests/shell/test_shell_interactive.py@494 PS13, Line 494: assert 'Fetched 1 row(s)' in result.stderr Same as above except using help, not select. http://gerrit.cloudera.org:8080/#/c/9933/13/tests/shell/test_shell_interactive.py@610 PS13, Line 610: ImpalaShellClass.strip_leading_comment('/*delete*/\n' ./tests/shell/test_shell_interactive.py:610:10: E127 continuation line over-indented for visual indent http://gerrit.cloudera.org:8080/#/c/9933/13/tests/shell/test_shell_interactive.py@619 PS13, Line 619: ImpalaShellClass.strip_leading_comment('/*delete*/\n' ./tests/shell/test_shell_interactive.py:619:10: E127 continuation line over-indented for visual indent -- 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: 13 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: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Taras Bobrovytsky <[email protected]> Gerrit-Comment-Date: Thu, 07 Jun 2018 17:22:22 +0000 Gerrit-HasComments: Yes
