David Knupp has posted comments on this change. ( http://gerrit.cloudera.org:8080/15524 )
Change subject: IMPALA-3343, IMPALA-9489: Make impala-shell compatible with python 3. ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/15524/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15524/4//COMMIT_MSG@17 PS4, Line 17: With regard to validating the patch, my assumption is that simply passing : the existing set of e2e shell tests is sufficient to confirm that the shell : is functioning properly. No new tests were added. > given that the main pain here was dealing with the encodings, would it make I actually think that we already have the necessary tests -- especially any tests that deal with international characters and test_unicode_input. I can't think now of other cases that would be required, but I'm happy to add tests if you can think of any additional scenarios. http://gerrit.cloudera.org:8080/#/c/15524/4//COMMIT_MSG@119 PS4, Line 119: No effort has been made at this point to come up with a way to integrate > do we have plans to do this? We do. I've haven't formally done anything yet with this, but it kind of falls under IMPALA-8373. http://gerrit.cloudera.org:8080/#/c/15524/4//COMMIT_MSG@121 PS4, Line 121: processes. > Maybe it's getting a new sqlparse version from somewhere? I don't think so. There's no change in the environment. I'm honestly not sure how this is working. http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py@600 PS4, Line 600: def precmd(self, args): : if sys.version_info.major == 2: : args = self.sanitise_input(args.decode('utf-8')) # python2 : else: > can we use the 'if sys.version_info.major == 2:' check here? Done http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py@1254 PS4, Line 1254: # Because the WITH clause may precede DML or SELECT queries, : # just checking the first token is insufficient. > was the first line suppose to be deleted? Oops, thanks. Done. http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py@1633 PS4, Line 1633: # In the case of a byte sequence, re.findall() will choke under python 3 > nit: could express as a single if with an "and" clause instead of nested if Ack http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py@1790 PS4, Line 1790: > I'm OK with deferring it, I don't know if it actually works on master. I do Ack http://gerrit.cloudera.org:8080/#/c/15524/4/shell/impala_shell.py@1913 PS4, Line 1913: # if the socket was interrupted, reconnect the connection with the client : if e.errno == errno.EINTR: : print(shell.CANCELLATION_MESSAGE) : shell._reconnect_cancellation() > what is this change for? I think it was a mistake -- thanks for catching it. http://gerrit.cloudera.org:8080/#/c/15524/4/tests/shell/test_shell_interactive.py File tests/shell/test_shell_interactive.py: http://gerrit.cloudera.org:8080/#/c/15524/4/tests/shell/test_shell_interactive.py@803 PS4, Line 803: we expect users to : # install 3rd upstream libraries from PyPI. > nit: revise Done -- To view, visit http://gerrit.cloudera.org:8080/15524 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb004d352fe230a890a6b6356496ba76c2fab615 Gerrit-Change-Number: 15524 Gerrit-PatchSet: 6 Gerrit-Owner: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 02 Apr 2020 16:58:48 +0000 Gerrit-HasComments: Yes
