Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15105 )
Change subject: IMPALA-5444: Asynchronous code generation ...................................................................... Patch Set 41: (6 comments) LGTM once the specific issues I mentioned are fixed. If you want to get a committer in a european timezone to carry my +2 and merge it after those are addressed, feel free. http://gerrit.cloudera.org:8080/#/c/15105/41/be/src/codegen/llvm-codegen.cc File be/src/codegen/llvm-codegen.cc: http://gerrit.cloudera.org:8080/#/c/15105/41/be/src/codegen/llvm-codegen.cc@1181 PS41, Line 1181: google::WARNING I think this should be 1, since the verbose levels and the ERROR/WARNING/INFO levels are not comparable. http://gerrit.cloudera.org:8080/#/c/15105/41/be/src/exprs/scalar-expr.inline.h File be/src/exprs/scalar-expr.inline.h: http://gerrit.cloudera.org:8080/#/c/15105/41/be/src/exprs/scalar-expr.inline.h@24 PS41, Line 24: /// TODO: Measure the effect of atomic loads. Remove this TODO http://gerrit.cloudera.org:8080/#/c/15105/40/be/src/util/tuple-row-compare.h File be/src/util/tuple-row-compare.h: http://gerrit.cloudera.org:8080/#/c/15105/40/be/src/util/tuple-row-compare.h@127 PS40, Line 127: /// TODO: Benchmark whether the atomic load is a performance issue (especially in > I haven't benchmarked this specifically but I have run a single node perf b OK, we can remove the TODO then.. http://gerrit.cloudera.org:8080/#/c/15105/41/common/thrift/ImpalaInternalService.thrift File common/thrift/ImpalaInternalService.thrift: http://gerrit.cloudera.org:8080/#/c/15105/41/common/thrift/ImpalaInternalService.thrift@430 PS41, Line 430: // TODO: Set default value to false after testing. Need to change this back before merging. http://gerrit.cloudera.org:8080/#/c/15105/40/tests/query_test/test_queries.py File tests/query_test/test_queries.py: http://gerrit.cloudera.org:8080/#/c/15105/40/tests/query_test/test_queries.py@56 PS40, Line 56: extend_exec_option_dimension(cls, "ASYNC_CODEGEN", 1) > I think I meant "ASYNC_CODEGEN=0", to test synchronous codegen in case asyn I think we can remove the TODO for now. http://gerrit.cloudera.org:8080/#/c/15105/40/tests/query_test/test_query_mem_limit.py File tests/query_test/test_query_mem_limit.py: http://gerrit.cloudera.org:8080/#/c/15105/40/tests/query_test/test_query_mem_limit.py@130 PS40, Line 130: # TODO: Test both synchronous and asynchronous codegen? In the case of async codegen > I added it because this test fails in async because for some reason we don' Can't we remove this in the final version of your patch though, if async_codegen=0 is the default there? Agree that it's probably just timing-related. -- To view, visit http://gerrit.cloudera.org:8080/15105 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia7cbfa7c6734dcf03641629429057d6a4194aa6b Gerrit-Change-Number: 15105 Gerrit-PatchSet: 41 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Wed, 24 Jun 2020 22:54:59 +0000 Gerrit-HasComments: Yes
