Pranav Lodha has posted comments on this change. ( http://gerrit.cloudera.org:8080/22419 )
Change subject: IMPALA-13705: Environment specific errors in test_encryption_exprs ...................................................................... Patch Set 1: (12 comments) > Patch Set 1: > > (2 comments) Thanks Daniel! http://gerrit.cloudera.org:8080/#/c/22419/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22419/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-13705 Please add some tests testing the first issue and a testing header. http://gerrit.cloudera.org:8080/#/c/22419/1/be/src/util/openssl-util.cc File be/src/util/openssl-util.cc: http://gerrit.cloudera.org:8080/#/c/22419/1/be/src/util/openssl-util.cc@221 PS1, Line 221: return Status("Mismatch between mode and key length."); We should add some test cases for this condition as well. http://gerrit.cloudera.org:8080/#/c/22419/1/be/src/util/openssl-util.cc@382 PS1, Line 382: OpenSSL implementation. Can we add the system configurations as well here in the error message? or be a bit descriptive as to why the mode is not supported. A reason as to why the mode is not supported would probably help in preventing confusion among the users about the actual modes supported. http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_128_ecb.test File testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_128_ecb.test: http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_128_ecb.test@3 PS1, Line 3: AES encryption/decryption examples: Please mention that its specific for AES_128_ECB mode here and also in the other files with their respective modes to maintain uniformity. I also feel that we should add some sort of uniformity in all the test files, for example, we should check for empty string and null cases in all the test files. http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_128_gcm.test File testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_128_gcm.test: http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_128_gcm.test@1 PS1, Line 1: ==== Would it make sense to mention in a comment about the specific openssl versions/ system configs where the mode in question is functional? I think it'll help bring more clarity. http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_128_gcm.test@38 PS1, Line 38: ---- QUERY : select count(*) from functional_parquet.alltypes where CAST(timestamp_col AS STRING) = : aes_decrypt(aes_encrypt(CAST(timestamp_col AS STRING), '1234567890123456', 'AES_128_GCM', '1234567890123456'), : '1234567890123456', 'AES_128_GCM', '1234567890123456'); I agree, similar tests should be added for all the modes. http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_256_ecb.test File testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_256_ecb.test: http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_aes_256_ecb.test@9 PS1, Line 9: ---- QUERY Please maintain uniformity in the other ecb file and include this case. http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_unsupported.test File testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_unsupported.test: http://gerrit.cloudera.org:8080/#/c/22419/1/testdata/workloads/functional-query/queries/QueryTest/encryption_exprs_unsupported.test@1 PS1, Line 1: ==== Please include some tests on the new cases that come up i.e. error tests that prevents fallback. http://gerrit.cloudera.org:8080/#/c/22419/1/tests/query_test/test_exprs.py File tests/query_test/test_exprs.py: http://gerrit.cloudera.org:8080/#/c/22419/1/tests/query_test/test_exprs.py@78 PS1, Line 78: test_encryption_exprs I also feel that the commit message can be a bit more elaborative, it can explain that we split the tests into these files, and some error tests in a separate file. http://gerrit.cloudera.org:8080/#/c/22419/1/tests/query_test/test_exprs.py@79 PS1, Line 79: Test handling encryption/ decryption functionality Please add a comment here about the need for separate test files. http://gerrit.cloudera.org:8080/#/c/22419/1/tests/query_test/test_exprs.py@83 PS1, Line 83: self.run_test_case('QueryTest/encryption_exprs_unsupported', vector) Please add a comment that it also includes the various errors that we can encounter. http://gerrit.cloudera.org:8080/#/c/22419/1/tests/query_test/test_exprs.py@108 PS1, Line 108: _check_aes_mode_supported Please add a comment explaining why this is required. -- To view, visit http://gerrit.cloudera.org:8080/22419 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I27146cda4fa41965de35821315738e5502d1b018 Gerrit-Change-Number: 22419 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Pranav Lodha <[email protected]> Gerrit-Comment-Date: Wed, 29 Jan 2025 17:26:30 +0000 Gerrit-HasComments: Yes
