Pranav Lodha has posted comments on this change. ( http://gerrit.cloudera.org:8080/20447 )
Change subject: IMPALA-13039: AES Encryption/ Decryption Support in Impala ...................................................................... Patch Set 31: (14 comments) > Uploaded patch set 31. 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: FunctionContext::FunctionStateScope scope) { > I wasn't really clear here, but I meant we could log the "No AES mode was s Done http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1826 PS29, Line 1826: context->G > If these functions are moved to the regular .cc file, we can switch back to Done http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1848 PS29, Line 1848: > I meant moving this initialisation after the check. In that case, we could Done http://gerrit.cloudera.org:8080/#/c/20447/29/be/src/exprs/string-functions-ir.cc@1862 PS29, Line 1862: > Using THREAD_LOCAL is leading to a failure in the tests: Done 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: context->GetFunctionState(FunctionContext::THREAD_LOCAL)); > Be careful with indentation: this and the corresponding ELSE block (startin Done http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc@1827 PS30, Line 1827: if (state != nullptr) { > Now we are not checking whether '*state' is true or false. Done http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc@1828 PS30, Line 1828: context->Free(reinterpret_cast<uint8_t*>(state)); > line too long (94 > 90) Done http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc@1891 PS30, Line 1891: > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc@1893 PS30, Line 1893: > "the default mode of the library" would be clearer. Done http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/exprs/string-functions-ir.cc@1895 PS30, Line 1895: > line too long (92 > 90) Done 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 Initialize Done http://gerrit.cloudera.org:8080/#/c/20447/30/be/src/util/openssl-util.cc@228 PS30, Line 228: RETURN_IF_ERROR(status); > We should return the error. If validation fails, we shouldn't continue. We Done 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@76 PS30, Line 76: > flake8: E303 too many blank lines (2) Done http://gerrit.cloudera.org:8080/#/c/20447/30/tests/query_test/test_exprs.py@77 PS30, Line 77: vector.get_value('exec_option')['enable_expr_rewrites'] = \ > This comment is not for this function. 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: 31 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 17:47:06 +0000 Gerrit-HasComments: Yes
