sunchao commented on code in PR #25491:
URL: https://github.com/apache/datafusion/pull/25491#discussion_r4087785560
##########
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);
+ let allowance = input_bytes + copy_bytes;
+ debug_assert!(retained <= allowance);
Review Comment:
Updated in
[cab63e46c7](https://github.com/apache/datafusion/commit/cab63e46c73014b5fc811e5976ee745be285c39b).
Restored a runtime error before shrinking the reservation. The guard is now
in `reconcile_prepared_copy_reservation`, so it remains active in release
builds. Its regression exercises an under-admitted retained size, verifies
`DataFusionError::Internal` and an unchanged reservation, and also covers exact
admission, a valid shrink, and final release.
All 14 prepared-build regressions pass in both the normal and optimized
release profiles.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -2944,7 +3037,21 @@ async fn collect_left_input(
// Arc is used instead of Box to allow sharing with
SharedBuildAccumulator for hash map pushdown
let mut hashmap = new_join_hashmap(num_rows, &mut reservation,
&metrics)?;
- let mut hashes_buffer = Vec::new();
+ let scratch_reservation = reservation.new_empty();
+ if prepared {
+ // Allow one logical null mask per key plus the combined mask.
+ let masks = if null_equality ==
NullEquality::NullEqualsNothing {
+ on_left.len() + 1
+ } else {
+ 0
+ };
+ scratch_reservation.try_grow(
+ max_batch_rows * size_of::<u64>()
+ + (max_batch_rows.div_ceil(8) + 64) * masks,
Review Comment:
Updated in
[cab63e46c7](https://github.com/apache/datafusion/commit/cab63e46c73014b5fc811e5976ee745be285c39b).
Agreed; I restored checked arithmetic for the prepared copy-size
accumulation, alignment allowance, scratch size, and final input/copy
allowance. The scratch bound still uses the simpler one-mask-per-key plus
combined-mask formula, and concat alignment is still charged once per output
buffer. Added boundary tests for multiplication and addition overflow,
including metadata-only NullArray batches so the tests need no large
allocations.
All 14 prepared-build regressions pass in both the normal and optimized
release profiles.
##########
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:
Updated in
[cab63e46c7](https://github.com/apache/datafusion/commit/cab63e46c73014b5fc811e5976ee745be285c39b).
I traced this and added a regression, but did not add the full-array charge
because it would count the retained key buffers twice. Prepared keys are
restricted to direct Columns: a single-key IN-list clones the same array Arc as
the build batch; a multi-key IN-list wraps the same column Arcs in a
StructArray. The final batch reservation therefore already covers their payload
buffers. The new test covers one/two keys and one/two input batches, verifies
Arc identity and zero additional unique buffers for membership, compares
IN-list and map reservations, and checks release after the last prepared lease.
The StructArray wrapper is metadata; the current batch accounting excludes
array/schema/Arc metadata generally. Consumer-local IN-list expression
allocations remain outside this reservation, as documented. I added a comment
beside the retained-buffer accounting to make the sharing explicit.
All 14 prepared-build regressions pass in both the normal and optimized
release profiles.
--
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]
