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 30: (9 comments) http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1823 PS29, Line 1823: bool* state = reinterpret_cast<bool*>( > Done I wasn't really clear here, but I meant we could log the "No AES mode was specified..." message if the state is NULL, not a different log message. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1826 PS29, Line 1826: x->AddWarn > Done If these functions are moved to the regular .cc file, we can switch back to VLOG_QUERY. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1848 PS29, Line 1848: if (!encryption_key->IsEcbMode() && iv.len > AES_BLOCK_SIZE) { > Done. Overloaded IsEcbMode() to accomodate it. I meant moving this initialisation after the check. In that case, we could check whether it's ECB using the 'cipher_mode' variable. http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc File be/src/exprs/string-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc@1826 PS30, Line 1826: ctx->AddWarning("Function state is NULL."); Be careful with indentation: this and the corresponding ELSE block (starting on 1827) should be indented more. http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc@1827 PS30, Line 1827: } else { Now we are not checking whether '*state' is true or false. http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc@1893 PS30, Line 1893: default mode "the default mode of the library" would be clearer. http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/util/openssl-util.cc@197 PS30, Line 197: Status ValidateModeAndKeyLength(AES_CIPHER_MODE m, int key_len) { We could also add a test that checks that InitializeRandom() and InitializeFields() return an error when they are called with a non-matching mode and key_len. http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/util/openssl-util.cc@228 PS30, Line 228: if (!status.ok()) { We should return the error. If validation fails, we shouldn't continue. We could use RETURN_IF_ERROR(Validate...)). This means that InitializeRandom() should return a Status. The same is true for InitializeFields(). http://gerrit.cloudera.org:8080/#/c/20447/30/tests/query_test/test_exprs.py File tests/query_test/test_exprs.py: http://gerrit.cloudera.org:8080/#/c/20447/30/tests/query_test/test_exprs.py@77 PS30, Line 77: """Test handling of expressions with "special" strings.""" This comment is not for this function. -- 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: 30 Gerrit-Owner: Pranav Lodha <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Fri, 24 Jan 2025 15:10:54 +0000 Gerrit-HasComments: Yes
