dwsmith1983 commented on code in PR #26793:
URL: https://github.com/apache/datafusion/pull/26793#discussion_r4430692931
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -3234,18 +3232,19 @@ fn build_in_memory(
}
// The maximum is known: avoid geometric growth and its excess
capacity.
let mut hashes_buffer = Vec::with_capacity(max_batch_rows);
- let mut offset = num_rows;
-
- // Update the hashmap starting from the last batch, so rows with
smaller
- // indices end up on top of the chains. Each batch is indexed at its
- // position in the forward concatenation below.
- for batch in batches.iter().rev() {
- offset -= batch.num_rows();
+ // `max_batch_rows` is 0 when the build side is empty
+ let chunk_rows = max_batch_rows.max(1);
+
+ // Update the hashmap from slices of at most `max_batch_rows` rows,
+ // starting from the last one, so rows with smaller indices end up on
+ // top of the chains. Each slice is indexed at its offset in `batch`.
+ for offset in (0..num_rows).step_by(chunk_rows).rev() {
+ let chunk = batch.slice(offset, chunk_rows.min(num_rows - offset));
Review Comment:
Thanks @Phoenix500526 for spotting this and @jayzhan211 for measuring it.
Applied in 09c18bf26: each slice is now at least as long as the longest key
dictionary, the prepared scratch reservation uses that length, and the hash
buffer capacity is capped at the build row count. The loop comment and the
`HashJoinExec` doc are updated to match.
One change to the test: with a separate dictionary per batch, `concat`
merges them down to the referenced values (3 here), which is no longer than the
largest batch, so the slicing stayed as before. The test now gives every build
batch one shared 5-value dictionary, which `concat` keeps as is, so the 12 rows
in 2-row batches are hashed in 5-row slices that cross batch boundaries.
Filling those slices front to back makes it fail, so it does reach the new path.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -107,8 +107,7 @@ const ARRAY_MAP_CREATED_COUNT_METRIC_NAME: &str =
"array_map_created_count";
/// (perfect hash join), returning the `(min, max)` key range to build it over.
///
/// On `Some`, the memory of the [`ArrayMap`] has already been added to
-/// `reservation`; the caller concatenates the build batches and creates the
map
-/// with [`ArrayMap::try_new`].
+/// `reservation`; the caller creates the map with [`ArrayMap::try_new`].
Review Comment:
Thanks. This is #26806, which @Phoenix500526 has taken and plans to pick up
once this PR merges, so I'll leave it for that.
--
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]
