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

Reply via email to