Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/20612 )
Change subject: IMPALA-3825: Delegate runtime filter aggregation to some executors ...................................................................... Patch Set 13: (2 comments) http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.cc File be/src/runtime/runtime-filter-bank.cc: http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.cc@139 PS10, Line 139: AllocateScratchBloomFilter > Filter aggregation can happen sometime later after all fragment start. I'm If the reservation is claimed, then it is considered a fatal error if allocating memory (up to the reservation) fails. https://github.com/apache/impala/blob/aeb9a8206028b68833ce6e49421990854f0c8ba4/be/src/runtime/bufferpool/buffer-pool.h#L77 Other queries should be only able to use the memory for buffers that can be dropped (e.g. spilled) http://gerrit.cloudera.org:8080/#/c/20612/10/be/src/runtime/runtime-filter-bank.cc@722 PS10, Line 722: result_filter->SetFilter(B > Agree. Done. Few things about this: - it is not clear to me whether this was a real bug, so whether not setting it could lead to dropping rows incorrectly - it would be nice to have a test for this scenario. Adding a delay/jitter debug action + low runtime_filter_wait_time_ms should be able to lead to the situation where all/some remote filters do not arrive in time. Looked around for tests like this and I couldn't find any that intentionally creates late filters. A cancellation related test seems to something like this: https://github.com/apache/impala/blob/b03e8ef95c856f499d17ea7815831e30e2e9f467/tests/query_test/test_runtime_filters.py#L110 I am ok moving test creation to a follow up Jira if it seems complicated. - do we actually need to run CombinePeerAndLocalUpdates? it seems unnecessary as the filter is all true - I don't think that we should put effort in optimizing this case, but a comment could mention that this is done for simplicity -- To view, visit http://gerrit.cloudera.org:8080/20612 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I11d38ed0f223d6e5b32a19ebe725af7738ee4ab0 Gerrit-Change-Number: 20612 Gerrit-PatchSet: 13 Gerrit-Owner: Riza Suminto <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Riza Suminto <[email protected]> Gerrit-Comment-Date: Wed, 13 Dec 2023 10:25:23 +0000 Gerrit-HasComments: Yes
