[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/21131 )
Change subject: IMPALA-11499: Refactor UrlEncode function to handle special characters ...................................................................... Patch Set 8: (15 comments) > Uploaded patch set 8: Commit message was updated. http://gerrit.cloudera.org:8080/#/c/21131/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21131/6//COMMIT_MSG@7 PS6, Line 7: IMPALA-11499: Refactor UrlEncode function to handle special characters > We don't usually break the title into two lines, and it is not too long eit Done http://gerrit.cloudera.org:8080/#/c/21131/6//COMMIT_MSG@8 PS6, Line 8: > nit: let's put the title in one line Done http://gerrit.cloudera.org:8080/#/c/21131/6//COMMIT_MSG@12 PS6, Line 12: bytes > Nit: surround with quotes ('specialCharacterMap'). Ack http://gerrit.cloudera.org:8080/#/c/21131/6//COMMIT_MSG@13 PS6, Line 13: 0xe8 0xbf 0x90 was wrongly getting encoded into '\E8%FFFFFFBF\90', > It's unclear to me how the unicode characters are handled incorrectly. E.g. Ack http://gerrit.cloudera.org:8080/#/c/21131/6//COMMIT_MSG@18 PS6, Line 18: '\xFF', which is an invalid UTF-8 byte is replaced > '*out' was assigned to also before this change, so its old contents were di Ack http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc File be/src/util/coding-util.cc: http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@23 PS6, Line 23: #include <boost/function.hpp> > I don't think we need iostream. Done http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@46 PS6, Line 46: ':', '=', '?', > Could also be static. Alternatively, we could put all these static function Done http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@55 PS6, Line 55: / cha > The problem in the old code was that "\u00FF" translates to two bytes: [194 Done http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@66 PS6, Line 66: } > No need to clear it if it is assigned to at the end of the function. Done http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@74 PS6, Line 74: : > Could be misleading: does "not" also apply to "one of the commonly..."? If Done http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@80 PS6, Line 80: > This is easier to understand if converted to a form without parentheses: Done http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@80 PS6, Line 80: > It's an existing issue but I think we should cast 'ch' to unsigned char as Done http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@85 PS6, Line 85: > nit: I think we can ignore explicitly using "std::" since std::uppercase is Done http://gerrit.cloudera.org:8080/#/c/21131/6/be/src/util/coding-util.cc@85 PS6, Line 85: > We could put std::uppercase and std::hex after L68, before the loop, as the Done http://gerrit.cloudera.org:8080/#/c/21131/3/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test File testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test: http://gerrit.cloudera.org:8080/#/c/21131/3/testdata/workloads/functional-query/queries/QueryTest/unicode-column-name.test@298 PS3, Line 298: > This comment has not been addressed. Done -- To view, visit http://gerrit.cloudera.org:8080/21131 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I88c4aba5d811dfcec809583d0c16fcbc0ca730fb Gerrit-Change-Number: 21131 Gerrit-PatchSet: 8 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zihao Ye <[email protected]> Gerrit-Comment-Date: Fri, 26 Apr 2024 15:51:23 +0000 Gerrit-HasComments: Yes
