Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/9933 )
Change subject: IMPALA-2195: Improper handling of comments in queries ...................................................................... Patch Set 7: (10 comments) After discussing with Fredy, I agree with his analysis that the current code is structured in a way that the query 'line' undergoes many transformations at multiple places and it probably does not make sense to store a global comment stripped query in the beginning and work on that. Adding to that there is a complexity of local shell commands (that aren't passed to the coordinator), which makes the code flow even more complex. So, the current approach of stripping the leading comments separately right before onecmd() is a reasonable approach and unfortunately, we do multiple query parsing attempts on the shell side on the same query which is something we need to fix eventually. Alex/Taras/others, do you have any reservations about this approach? 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 leading comments or something like that. http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@322 PS7, Line 322: self.comment if self.comment else "" Can be simplified with -- self.comment or "" http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@590 PS7, Line 590: command, arg, line, comment = self.parseline(line) can we call it prefix_comments/leading_comments to make it more clearer? comment seems too general. http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1288 PS7, Line 1288: """Parse the line into a command name and a string containing I think we need to expand the method doc a little more. Why we strip the leading comment. Please add an example if you think that helps. Also how we deal with local commands that aren't sent to the fe. http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1289 PS7, Line 1289: ). , comment http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1294 PS7, Line 1294: produce a 'command' correctly nit:reword to ..deduce the 'command' part ... maybe? http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1295 PS7, Line 1295: This function returns a tuple(command, arg, line, comment) nit: Already in the beginning, keep one. http://gerrit.cloudera.org:8080/#/c/9933/7/shell/impala_shell.py@1306 PS7, Line 1306: Filter a leading comment in the SQL statement. nit: Returns (comment, query.....? I'm hoping this either makes it to the sqlstream code or we move it to shell/ext-py/sqparse.. as a patch. Seems a little out of place here. Thoughts? 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. http://gerrit.cloudera.org:8080/#/c/9933/7/tests/shell/test_shell_interactive.py@565 PS7, Line 565: Can you add one example with join hints too? SHUFFLE etc. -- 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: 7 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 19:26:05 +0000 Gerrit-HasComments: Yes
