Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/9928 )
Change subject: IMPALA-2717: fix output of formatted unicode to non-TTY ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/9928/1/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/9928/1/shell/impala_shell.py@73 PS1, Line 73: """Patched version of PrettyTable that TODO""" Not your change, but would you mind filling in the TODO? The difference seems to be that we're passing "replace" instead of "strict" in /usr/local/lib/python2.7/dist-packages/prettytable.py:181 . http://gerrit.cloudera.org:8080/#/c/9928/1/shell/shell_output.py File shell/shell_output.py: http://gerrit.cloudera.org:8080/#/c/9928/1/shell/shell_output.py@45 PS1, Line 45: row > It's already a str (verified by printing type(rows[0][0])). I looked around a bit and tried to figure out how passing a str into add_row() can result in unicode coming out of get_string(). PrettyTable calls _unicode() on the row's fields, which will encode the str() back into unicode. Do you want to add a brief comment mentioning this conversion inside add_row, maybe before L35? http://gerrit.cloudera.org:8080/#/c/9928/2/shell/shell_output.py File shell/shell_output.py: http://gerrit.cloudera.org:8080/#/c/9928/2/shell/shell_output.py@31 PS2, Line 31: containe nit: containing? http://gerrit.cloudera.org:8080/#/c/9928/2/shell/shell_output.py@58 PS2, Line 58: contained here too? -- To view, visit http://gerrit.cloudera.org:8080/9928 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9de641ecf767a2feef3b9f48b344ef2d55e17a7f Gerrit-Change-Number: 9928 Gerrit-PatchSet: 1 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 11 Apr 2018 21:37:35 +0000 Gerrit-HasComments: Yes
