Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/17170 )
Change subject: IMPALA-7825: Upgrade Thrift version to 0.11.0 ...................................................................... Patch Set 23: (7 comments) Thanks a lot Quanlong for the detailed analysis! I added more conversions, and now test_shell_interactive.py passes with the non-accelerated protocol. I like the code less and less though and become unsure about the no_utf8strings option. When reading thrift structures, it makes sense, as we can avoid unnecessary decode + encode pairs if we expect the result in utf8. But when writing, it would be better to convert every 'unicode' to utf8, it too much hassle to do this in the caller. I think that ideally Thrift would always encode when writing but return string during read based on some option from the protocol, and do this consistently in both accelerated and normal protocol. http://gerrit.cloudera.org:8080/#/c/17170/21/shell/impala-shell File shell/impala-shell: http://gerrit.cloudera.org:8080/#/c/17170/21/shell/impala-shell@29 PS21, Line 29: 0.1 > stale comment Done http://gerrit.cloudera.org:8080/#/c/17170/21/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/17170/21/shell/impala_client.py@85 PS21, Line 85: # Helper to decode utf8 encoded str to unicode type in Python 2. NOOP in Python 3. > While calling this on all string fields from thrift, I think we also need t Done http://gerrit.cloudera.org:8080/#/c/17170/21/shell/impala_client.py@735 PS21, Line 735: > I think we need to encode this into 'str' when it's 'unicode' in python2. T Done http://gerrit.cloudera.org:8080/#/c/17170/21/shell/impala_client.py@736 PS21, Line 736: ngImpalaHS2Service rpc is ide > This also contains unicodes, which could lead to an error in ImpalaHttpClie Done http://gerrit.cloudera.org:8080/#/c/17170/21/shell/impala_client.py@1120 PS21, Line 1120: _service( > I think we need to encode this too, if it's unicode in python2. Done http://gerrit.cloudera.org:8080/#/c/17170/22/shell/impala_client.py File shell/impala_client.py: http://gerrit.cloudera.org:8080/#/c/17170/22/shell/impala_client.py@85 PS22, Line 85: # Helper to decode utf8 encoded str to unicode type in Python 2. NOOP in Python 3. > flake8: E302 expected 2 blank lines, found 1 Done http://gerrit.cloudera.org:8080/#/c/17170/22/shell/impala_client.py@91 PS22, Line 91: > flake8: E302 expected 2 blank lines, found 1 Done -- To view, visit http://gerrit.cloudera.org:8080/17170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idd13f177b4f7acc07872ea6399035aa180ef6ab6 Gerrit-Change-Number: 17170 Gerrit-PatchSet: 23 Gerrit-Owner: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Qifan Chen <qc...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Reviewer: Tamas Mate <tm...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Mon, 26 Apr 2021 16:34:04 +0000 Gerrit-HasComments: Yes
