Qifan Chen has posted comments on this change. ( http://gerrit.cloudera.org:8080/19181 )
Change subject: IMPALA-11470: Add Cache For Codegen Functions ...................................................................... Patch Set 14: Code-Review+1 (15 comments) Some additional comments to the commit message. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@13 PS14, Line 13: save nit avoid repeated http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@14 PS14, Line 14: . It is also useful to mention whether the cache is per process or per backend. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@16 PS14, Line 16: in a fragment-level way nit at the fragment level. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@18 PS14, Line 18: one nit you mean another? http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@34 PS14, Line 34: Codegen cache is disabled if a fragment uses a native udf, because nit automatically http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@41 PS14, Line 41: . May file a JIRA and mention the JARA number here. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@43 PS14, Line 43: flags for start and query : options nit query options for feature configuration and operation purposes. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@45 PS14, Line 45: start option nit Configuration. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@48 PS14, Line 48: : query option nit Operations http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@50 PS14, Line 50: - disable_codegen_cache : Codegen cache will be disabled when it is set to true. nit suggest to indent as follows for readability. query option: - disable_codegen_cache: codegen cache will be disabled when it is set to true. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@61 PS14, Line 61: store the hash code I wonder if we can also store the length of the full key so that we can reject a good portion of collision cases when lengths differ. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@70 PS14, Line 70: impala.codegen-cache.misses : impala.codegen-cache.entries-in-use : impala.codegen-cache.entries-in-use-bytes : impala.codegen-cache.entries-evicted : impala.codegen-cache.hits : impala.codegen-cache.entry-sizes nit. indent http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@78 PS14, Line 78: CodegenCacheLookupTime : CodegenCacheSaveTime : ModuleBitcodeGenTime : NumCachedFunctions nit indent http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@86 PS14, Line 86: Delta(Avg) It might be informational to add a column showing the cache overhead in bytes. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@113 PS14, Line 113: cache is not always faster When codegen is disabled, how can codegen cache be still useful? -- 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: 14 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: Thu, 01 Dec 2022 17:09:02 +0000 Gerrit-HasComments: Yes
