Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19181 )
Change subject: IMPALA-11470: Add Cache For Codegen Functions ...................................................................... Patch Set 11: (5 comments) http://gerrit.cloudera.org:8080/#/c/19181/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19181/9//COMMIT_MSG@34 PS9, Line 34: Codegen cache is disabled if a fragment uses a native udf, because > Thanks Csaba and Daniel. Thanks for investigating this! http://gerrit.cloudera.org:8080/#/c/19181/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19181/11//COMMIT_MSG@53 PS11, Line 53: codegen_cache_mode What will happen if the user changes between the normal and optimal? This cannot cause collusions as the optimal is always shorter than a normal, right? http://gerrit.cloudera.org:8080/#/c/19181/11/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/19181/11/be/src/codegen/llvm-codegen.cc@932 PS11, Line 932: fn.binary_type == TFunctionBinaryType::NATIVE My understanding is that this can be false only in case broken_builtin == true - can you add a DCHECK for that and also an EE test that uses one of these functions? Note that I am not sure whether this broken_builtin hack is still needed. http://gerrit.cloudera.org:8080/#/c/19181/11/common/thrift/Query.thrift File common/thrift/Query.thrift: http://gerrit.cloudera.org:8080/#/c/19181/11/common/thrift/Query.thrift@90 PS11, Line 90: enum TCodeGenCacheMode { > Ack I don't have a strong opinion on the query options - generally the ones we expect user to fiddle with should be understandable and preferably backwards compatible. The debug ones can be created in the simplest way for the developer and changed if needed. http://gerrit.cloudera.org:8080/#/c/19181/11/tests/custom_cluster/test_codegen_cache.py File tests/custom_cluster/test_codegen_cache.py: http://gerrit.cloudera.org:8080/#/c/19181/11/tests/custom_cluster/test_codegen_cache.py@51 PS11, Line 51: test_codegen_cache_int_col optional: I think that it could make the test s bit faster if these similar tests were merged to a single test -- To view, visit http://gerrit.cloudera.org:8080/19181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If42c78a7f51fd582e5fe331fead494dadf544eb1 Gerrit-Change-Number: 19181 Gerrit-PatchSet: 11 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[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: Michael Smith <[email protected]> Gerrit-Reviewer: Qifan Chen <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Tue, 29 Nov 2022 16:01:09 +0000 Gerrit-HasComments: Yes
