Daniel Becker 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 12: (6 comments) Thanks Pranav, we're close. http://gerrit.cloudera.org:8080/#/c/21131/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21131/12//COMMIT_MSG@15 PS12, Line 15: ' Use double quotes here ("\u00FF"), this is not a valid character literal but it is a valid string literal. Same also later on this line. http://gerrit.cloudera.org:8080/#/c/21131/12//COMMIT_MSG@18 PS12, Line 18: the special characters "the special characters that need to be escaped" would be better, it's not all special chars. http://gerrit.cloudera.org:8080/#/c/21131/12//COMMIT_MSG@18 PS12, Line 18: , Nit: no need for the comma here. http://gerrit.cloudera.org:8080/#/c/21131/12//COMMIT_MSG@18 PS12, Line 18: included I think it could be formulated better. There are two things that have changed: 1. '\xFF' was corrected to '\x7F' and 2. before this change we also had a "set" of characters that needed to be encoded, but it used to be stored as a string and now it's stored as an unordered_set (which is much better in my opinion). I think we should mention these separately. http://gerrit.cloudera.org:8080/#/c/21131/12/be/src/util/coding-util.cc File be/src/util/coding-util.cc: http://gerrit.cloudera.org:8080/#/c/21131/12/be/src/util/coding-util.cc@19 PS12, Line 19: #include <cctype> Let's fix include groups: there should be an empty line after #include "util/coding-util.h": after it come the standard library includes. Also, leave a line after <unordered_set> because the boost and sasl includes are includes from other libraries than std. Usually we group includes like this: 1. header belonging to the current .cc file 2. standard library includes 3. other (third party) library includes 4. impala includes Each group is ordered alphabetically and there is an empty line between groups. http://gerrit.cloudera.org:8080/#/c/21131/12/be/src/util/coding-util.cc@50 PS12, Line 50: '\x0C' This could be '\f' (form feed), see https://www.compart.com/en/unicode/U+000C. -- 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: 12 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: Tue, 30 Apr 2024 10:36:08 +0000 Gerrit-HasComments: Yes
