2010YOUY01 commented on code in PR #26793:
URL: https://github.com/apache/datafusion/pull/26793#discussion_r4431003552
##########
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:
I think it's an unnecessary extra guarantee that I'd prefer to avoid to keep
the implementation simple.
Fundamentally, dictionary encoding is an opt-in optimization rather than a
general-purpose encoding. It's only efficient when cardinality is low. So a
better design would be that any operator producing dictionary output should
ensure it has low cardinality.
This fix takes a different assumption: an operator can produce
high-cardinality dictionary output, and all downstream operators must
defensively handle it. I think this unnecessarily propagates complexity to all
downstream operators.
Question: Do we have a written convention for such cases? I think it's
important to establish one to prevent similar complexity from being introduced
in the future.
--
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]
