Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 17: (17 comments) http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@1991 PS17, Line 1991: encryption_key. style nit (if applicable to Impala's code): consider an explicit notation of calling static methods, i.e. EncryptionKey::GetSupportedDefaultMode() http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@1993 PS17, Line 1993: .ModeToString ditto http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@1995 PS17, Line 1995: .StringToMode ditto http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2088 PS17, Line 2088: encryption_key. style nit (if applicable to Impala's code): consider an explicit notation of calling static methods, i.e. EncryptionKey::ModeToString(...) http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2090 PS17, Line 2090: encryption_key.StringToMode style nit: ditto http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions-ir.cc@2094 PS17, Line 2094: if(cipher_mode == AES_CIPHER_MODE::INVALID) { nit: missing a space after 'if' and before the parenthesis http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h File be/src/exprs/string-functions.h: http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@139 PS17, Line 139: nit: remove an extra space? http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@140 PS17, Line 140: ditto http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@140 PS17, Line 140: ditto http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@142 PS17, Line 142: dito http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/exprs/string-functions.h@142 PS17, Line 142: ditto http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h File be/src/util/openssl-util.h: http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@194 PS17, Line 194: void GetGcmTag(uint8_t* out) nit: could this be void GetGcmTag(uint8_t* out) const? Also, consider documenting this new method, similar to the rest of them in this interface. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.h@195 PS17, Line 195: uint8_t* nit: could this be 'const uint8_t*'? Also, consider documenting this new method, similar to the rest of them in this interface. http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/util/openssl-util.cc@407 PS13, Line 407: } else if (strncmp("AES_256_CTR", str, len) == 0) > It throws this error, so didn't change it: An option might be dropping the custom clause for 'AES_CIPHER_MODE::INVALID' and adding 'default' that would return "INVALID" string. With that, it could be possible to remove the 'return "INVALID";' at line 409 as of PS13. http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc@375 PS17, Line 375: case AES_CIPHER_MODE::INVALID: nit: this line might be removed http://gerrit.cloudera.org:8080/#/c/20447/17/be/src/util/openssl-util.cc@404 PS17, Line 404: AES_CIPHER_MODE EncryptionKey::StringToMode(const char* str, int len) { >From the usability and ease-of-use perspective, does it make sense to use >case-insensitive string comparison in this method? http://gerrit.cloudera.org:8080/#/c/20447/13/common/function-registry/impala_functions.py File common/function-registry/impala_functions.py: http://gerrit.cloudera.org:8080/#/c/20447/13/common/function-registry/impala_functions.py@520 PS13, Line 520: [[ > nit: remove extra spaces? Did you miss addressing this nit or the indent is supposed to be like this? -- 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: 17 Gerrit-Owner: Pranav Lodha <[email protected]> Gerrit-Reviewer: Alexey Serbin <[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-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Sat, 24 Aug 2024 00:04:51 +0000 Gerrit-HasComments: Yes
