David Knupp 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) Patch 13 will address all of the comments on patch set 12. 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? Thrift >= 11 is required for python 3, and so USE_THRIFT11_GEN_PY must be true when we build the package or test the shell in a python3 environment. In fact, we really need to enforce this, not just take it on faith, so my next patch will address that. 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 I'm not sure about this -- there's probably some more cleanup that we could do. I initially tried to follow the technique of the so-called "unicode sandwich" (decode early, encode at the edge), but I was still seeing some failures. It's possible I missed something, or that we have an inherently unstable situation, testing with python 2.x in all cases, but against a shell that might be running with either python 2 or 3. I'm open to trying out any specific suggestions you might have. 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 pyth Yeah -- there's almost so much information online on this topic that it's hard to sift through. This seems like a good place to start: https://medium.com/better-programming/strings-unicode-and-bytes-in-python-3-everything-you-always-wanted-to-know-27dc02ff2686 This slide deck is frequently referenced: https://nedbatchelder.com/text/unipain/unipain.html#1 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 I gave it a shot. Turns out that redefining built-ins outright is allowed, but importing is not, so I had to do a bit of renaming. -- 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: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 25 Feb 2020 01:49:25 +0000 Gerrit-HasComments: Yes
