Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 13: (3 comments) http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/13/be/src/exprs/string-functions-ir.cc@1810 PS13, Line 1810: ECB mode > I've added a note under AES-ECB. I meant this part: > Also, encryption in ECB mode is not supported, so it is a bit > confusing to list here what parameters are necessary for ECB > encryption. > I agree that it is useful to include a detailed description about > the modes. I would put this long comment above the aes_decrypt > function since that is supported for all modes. That way the > comment aligns better with the code. > And just a suggestion: you can switch the order of encrypt and > decrypt functions Changing the order of aes_encrypt() and aes_decrypt() is a good idea. http://gerrit.cloudera.org:8080/#/c/20447/17/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/20447/17/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3586 PS17, Line 3586: > The key is not NULL here. Sorry, not the key but the encryption mode. Why isn't the empty string invalid? http://gerrit.cloudera.org:8080/#/c/20447/17/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3601 PS17, Line 3601: > Its taking the fallback mode, why is it not okay? We should test the AES modes explicitly. The fallback mode may change in the future and then suddenly we are testing another mode than before. Also, the tests will fail. Also, in general, I don't think using the encoding/decoding functions without an explicit mode is good practice, precisely because the default may change and because different DB engines may have different defaults. -- 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: 13 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: Wed, 28 Aug 2024 13:30:11 +0000 Gerrit-HasComments: Yes
