Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 29: (5 comments) Joining this review late - I didn't have capacity to read all previous comments, sorry if some of the topics were already touched. The change looks great in general, though I would prefer some code to be moved to different files. 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@1862 PS29, Line 1862: if (scope != FunctionContext::FRAGMENT_LOCAL) return; This doesn't look correct to me - as the state is mutable, we should use THREAD_LOCAL instead of FRAGMENT_LOCAL state. While this shouldn't cause problems with a such simple state, strictly speaking it can still lead to a race condition. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1910 PS29, Line 1910: StringVal StringFunctions::aes_decrypt(FunctionContext* ctx, const StringVal& expr, I would prefer to move most of the new code out of the ir.cc - see GetJsonObjectImpl() for an example that where there is only stub here and the actual logic is in string-functions.cc There are few reasons for this: - as these are relatively slow functions I don't think that we win anything by inlining them during codegen - this file is already pretty large - I am concerned about including openssl headers in cross compiled files, as this may bloat the bytecode files that we have to process during codegen. http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/util/openssl-util.cc@269 PS29, Line 269: success = encrypt ? EVP_EncryptInit_ex(ctx.ctx, nullptr, nullptr, key_, iv_): : EVP_DecryptInit_ex(ctx.ctx, nullptr, nullptr, key_, iv_); fyi I uploaded a similar small change to set IV after setting the length: https://gerrit.cloudera.org/#/c/22337/ The solution is nearly the same, so it should only cause a minor conflict. I plan to merge it quickly to fix Impala with newer OpenSsl versions. http://gerrit.cloudera.org:8080/#/c/20447/29/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: http://gerrit.cloudera.org:8080/#/c/20447/29/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3291 PS29, Line 3291: # AES encryption/decryption examples: I would prefer to move these tests to a new file, e.g. encryption_exprs.test and call it from a new test in test_exprs.py I are few reasons for this: - this file is already very large - encrypt/decrypt functions are likely to break for very specific reasons, e.g. openssl version/configuration differences between OSs - this means that we may need to add OS/OpenSsl specific skipIfs in the future - different test parameter vectors may be useful for encryption functions than for regular functions http://gerrit.cloudera.org:8080/#/c/20447/29/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3299 PS29, Line 3299: select I don't see any test that has a FROM clause, so each test is evaluated simply in the coordinator. It would be better to also evaluate the functions in an executor/scanner context. An example that would call this encrypt/decrypt in the scanner is select count(*) from functional.alltypes where string_col = aes_decrypt(aes_encrypt(string_col, <key>, <mode>, <iv>)) , <key>, <mode>, <iv>). (expected result is 7300). It could also extend coverage to get key/mode/iv from a table, e.g create a table with some example modes (+key+iv), and also add some data or join it with another table. -- 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: 29 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: Tue, 14 Jan 2025 14:40:21 +0000 Gerrit-HasComments: Yes
