Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15132 )
Change subject: IMPALA-3343: Make impala-shell compatible with python 3. ...................................................................... Patch Set 12: (4 comments) I did a pass over the change and had some high level questions/suggestions. I haven't focused yet on the string-handling part. http://gerrit.cloudera.org:8080/#/c/15132/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15132/12//COMMIT_MSG@39 PS12, Line 39: USE_THRIFT11_GEN_PY=true impala-py.test --shell_executable=/<path to virtualenv>/bin/impala-shell -sv shell/test_shell_commandline.py Is thrift 11 a requirement for python 3 support? http://gerrit.cloudera.org:8080/#/c/15132/12//COMMIT_MSG@115 PS12, Line 115: What ultimately seemed like a better approach was to try to weed out as many Is there any way to document or enforce that the strings in the code are a particular type of string? It might help at least to ensure that some parts of the shell code are clean. http://gerrit.cloudera.org:8080/#/c/15132/12//COMMIT_MSG@116 PS12, Line 116: existing spurious str.encode() and str.decode() calls as I could. I'm not sure Also, do you have any references to best practices or other info about python string handling? http://gerrit.cloudera.org:8080/#/c/15132/12/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/15132/12/shell/impala_shell.py@51 PS12, Line 51: try: Would it be worth factoring out some of these compatibility patterns into a separate file? E.g. you could import like this. from compat import xrange, basestring It might be overkill, but it did occur to me while reading through -- To view, visit http://gerrit.cloudera.org:8080/15132 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibb75e162bac0faeae3e12106c15da39cbfb8b462 Gerrit-Change-Number: 15132 Gerrit-PatchSet: 12 Gerrit-Owner: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 13 Feb 2020 17:03:08 +0000 Gerrit-HasComments: Yes
