[email protected] has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-12418: Include crypto functions supported by Hive ...................................................................... Patch Set 6: (38 comments) > Uploaded patch set 6. http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@40 PS3, Line 40: #include "util/coding-util.h" : #include "util/opens > The includes should be in alphabetical order. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@43 PS3, Line 43: #include "util/string-util.h" : #include "util/ubsan.h" : #include "util/url-parser.h" : > Put these together with the other std library includes (L20), in alphabetic Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@182 PS3, Line 182: "mul > Unnecessary spaces. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@551 PS3, Line 551: ic_a > Unnecessary spaces. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@854 PS3, Line 854: n UTF8 mode, po > Unnecessary spaces. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1076 PS3, Line 1076: > Unnecessary spaces. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1112 PS3, Line 1112: _ptr<re > Unnecessary spaces. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1344 PS3, Line 1344: > Unnecessary spaces. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1624 PS3, Line 1624: > Unnecessary spaces. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1625 PS3, Line 1625: le m = static_cast<double > Unnecessary spaces. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1806 PS3, Line 1806: > Could be const. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1806 PS3, Line 1806: > Why call it 'ctx1'? It could simply be 'ctx'. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1806 PS3, Line 1806: turn IntVal > Add a doc comment describing what this function does, especially what 'ecb' Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1807 PS3, Line 1807: > Could be const. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1807 PS3, Line 1807: > Nit: add space after the comma. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1808 PS3, Line 1808: : // Electronic > No need to declare these here. 'len' could be done just before L1823, and ' Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1811 PS3, Line 1811: on > This could be 'cipher_ctx'. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1811 PS3, Line 1811: each block of inpu > We should check whether we can reuse the EVP_CIPHER_CTX between calls to th We can probably try calling a constructor but that'd just lead to addition of a new function, which I don't think would be desirable. http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1814 PS3, Line 1814: nc > Do we need the newline at the end of error messages? Applies to all other e Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1818 PS3, Line 1818: re D > Use 'nullptr' instead of 'NULL'. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1818 PS3, Line 1818: es t > Use 'nullptr' instead of 'NULL'. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1836 PS3, Line 1836: EVP_CIPHER_CTX_free(cipher_ctx); > We have some early returns in the function, in which case this "free" funct Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1841 PS3, Line 1841: dat > See L1806. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1841 PS3, Line 1841: > Could be const. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1842 PS3, Line 1842: (!EVP_EncryptU > Could be const. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1842 PS3, Line 1842: t > We usually put the '*' on the type, not the variable name. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1853 PS3, Line 1853: ; > Use 'nullptr' instead of 'NULL'. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1853 PS3, Line 1853: () f > Use 'nullptr' instead of 'NULL'. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1871 PS3, Line 1871: return -1; > See L1836. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1884 PS3, Line 1884: x->SetE > I don't think extracting 'key.len' into a variable is useful. Could simply Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1885 PS3, Line 1885: P_CIPHER_ > See L1884 about 'key_len'. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1886 PS3, Line 1886: > Surround '%' with spaces. A comment should explain why we have this length. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1887 PS3, Line 1887: > No need for this vector, could simply use the memory of 'input', i.e. "inpu Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1889 PS3, Line 1889: er_len > No need for this vector, could simply use the memory of 'key', i.e. "key.pt Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1904 PS3, Line 1904: > Use 'nullptr' instead of 'NULL'. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1912 PS3, Line 1912: > We should add a DCHECK for the specific case where "ret_size > output_len" If this is what you meant, it leads to a failure of the tests. http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1920 PS3, Line 1920: case 16: > See comments for aes_encrypt() above, they apply also here. Done http://gerrit.cloudera.org:8080/#/c/20447/3/be/src/exprs/string-functions-ir.cc@1954 PS3, Line 1954: > In the case of decryption, this copy may be worth keeping because the resul Done -- To view, visit http://gerrit.cloudera.org:8080/20447 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3902f2b1d95da4d06995cbd687e79c48e16190c9 Gerrit-Change-Number: 20447 Gerrit-PatchSet: 6 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: Joe McDonnell <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Comment-Date: Fri, 19 Jan 2024 06:31:20 +0000 Gerrit-HasComments: Yes
