Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19181 )
Change subject: IMPALA-11470: Add Cache For Codegen Functions ...................................................................... Patch Set 15: (14 comments) 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: avoi > nit avoid repeated Done 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 backen Added comments. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@16 PS14, Line 16: store codegen function > nit at the fragment level. Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@18 PS14, Line 18: ntr > nit you mean another? Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@34 PS14, Line 34: for the fragment if using a dynamic hash seed within the codegen > nit automatically Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@41 PS14, Line 41: > May file a JIRA and mention the JARA number here. Filed IMPALA-11771 for it. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@43 PS14, Line 43: could lead to a crash due : to usin > nit query options for feature configuration and operation purposes. Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@45 PS14, Line 45: is a better > nit Configuration. Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@48 PS14, Line 48: options for feature configuration and operation purpose. : Start option > nit Operations Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@50 PS14, Line 50: - codegen_cache_capacity: The capacity of the cache, if set to 0, : codegen cache is disabled. > nit suggest to indent as follows for readability. Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@61 PS14, Line 61: e stored to the cac > I wonder if we can also store the length of the full key so that we can rej I think it is a good idea, and changed the optimal mode to use hash code plus length, passed the tests, and should be good. http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@70 PS14, Line 70: Only valid when disable_codegen_cache is set to false. : : New impalad metrics: : - impala.codegen-cache.misses : - impala.codegen-cache.entries-in-use : - impala.codegen-cache.entries > nit. indent Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@78 PS14, Line 78: - impala.codegen-cache.entry-sizes : : New profile Metrics: : - CodegenCacheLo > nit indent Done http://gerrit.cloudera.org:8080/#/c/19181/14//COMMIT_MSG@113 PS14, Line 113: From the result, it shows > When codegen is disabled, how can codegen cache be still useful? I think the case mentioning here is to compare codegen disabled vs codegen and codegen cache enabled. Because there are some options could disable the codegen in runtime, even though codegen is enbabled in the daemon, for example disable_codegen_rows_threshold. Because for short queries, codegen disabled is always faster nowaday, one task for codegen cache is to replace this kind of codegen disabled, but it needs to be faster than it. -- 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: 15 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: Fri, 02 Dec 2022 04:18:19 +0000 Gerrit-HasComments: Yes
