David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/9195 )
Change subject: IMPALA-6337: Fix infinite loop in Impala shell ...................................................................... Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@588 PS9, Line 588: for index in xrange(0, len(parsed_statements)): > It probably is faster, but I don't think performance should be a concern in For what it's worth, I think enumerate is either as fast or faster than xrange. Python built-ins tend to be highly optimized. And either way, I think that this is clearly a case where the benefit of enhanced readability far outweighs any infinitesimal performance gains. http://gerrit.cloudera.org:8080/#/c/9195/9/shell/impala_shell.py@601 PS9, Line 601: if not found_error: > After spending some time think about this issue, I don't think there's an e The problem seems to be in the stated assumption: "Error tokens are assumed to be generated as pairs." Rather than trying to workaround the issue, I wonder if this may be a case where the best course of action is not to try to infer the intent of malformed SQL, but rather to bail gracefully with explicit messages. E.g., one could add something like: @staticmethod def is_valid_sql(sql): """Use sqlparse module to validate that sql doesn't contain any errors. Returns: True if no errors found, else return False. """ parsed = sqlparse.parse(sql) for parsed_stmt in parsed: if any(tok.ttype == sqlparse.tokens.Error for tok in parsed_stmt.tokens): return False return True And then use this in precmd() like: parsed_cmds = sqlparse.split(args) if not all(ImpalaShell.is_valid_sql(cmd) for cmd in parsed_cmds): # Probably output args + a message to STDOUT, and then take # action appropriate to whether self.interactive is True or False else: if len(parsed_cmds) > 1: # etc... -- To view, visit http://gerrit.cloudera.org:8080/9195 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9142f21a888189d351f00ce09baeba123bc0959b Gerrit-Change-Number: 9195 Gerrit-PatchSet: 10 Gerrit-Owner: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 07 Mar 2018 19:19:44 +0000 Gerrit-HasComments: Yes