jayzhan211 commented on code in PR #25491:
URL: https://github.com/apache/datafusion/pull/25491#discussion_r4082486429
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -3104,18 +3211,28 @@ async fn collect_left_input(
&& !left_values.is_empty()
&& left_values[0].logical_null_count() > 0;
+ if prepared {
+ drop(batches);
+ let retained = RecordBatchMemoryCounter::new().count_batch(&batch);
Review Comment:
For non-byte keys, `membership` can be `PushdownStrategy::InList(values)`.
That array lives in `JoinBuildData` for the build's whole lifetime and is
shared by every consumer. But the final shrink keeps the charge only for
`batch` (the map was charged earlier), so the IN-list is never charged, even
though the docs say preparation charges retained data. Each uncharged array is
limited by `hash_join_inlist_pushdown_max_size`, which is configurable. Fine to
handle in a follow-up.
```diff
if prepared {
drop(batches);
+ if let PushdownStrategy::InList(values) = &membership {
+ reservation.try_grow(values.get_array_memory_size())?;
+ }
let retained = RecordBatchMemoryCounter::new().count_batch(&batch);
```
A test would help: prepare an Int64 build that picks `InList`, then assert
`prepared.reserved_bytes()` is at least the batch count plus the map estimate
plus the IN-list array size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
