viirya commented on PR #25491:
URL: https://github.com/apache/datafusion/pull/25491#issuecomment-5740085816
Catching up on @comphead's and @jayzhan211's reviews, which landed while I
was reading. I checked the two claims that bear on what I wrote, and both hold
— with one caveat on the `GetSlicedSize` migration that I think matters.
**`new_join_hashmap` under-reserves on `main` today** (@comphead's note on
`exec.rs:3039`) — confirmed against `upstream/main` @ `a522cd5`.
`JoinHashMapU32::with_capacity` allocates `next: vec![0; cap]`
(`join_hash_map.rs:160`), and `estimate_memory_size` covers only
`size_of::<T>() * buckets + buckets + fixed_size`
(`common/src/utils/memory.rs:105-116`) — no chain term, and no later `try_grow`
for it. So every `CollectLeft` and `Partitioned` build under-reserves `num_rows
* 4` bytes, `* 8` above `u32::MAX`, on `main` right now.
That reframes my B2 comment. I read the `if prepared` row-index `try_grow`
as prepared-only bookkeeping and suggested softening the `internal_err!` around
it. That was the wrong altitude: this is a pre-existing accounting bug that the
prepared path happens to fix for itself. Moving it into `new_join_hashmap`
fixes every hash join and deletes the special case. Worth splitting into its
own PR so it can be backported independently of this feature.
**`prepared_copy_bytes` duplicating `ArrayData::get_slice_memory_size`**
(`prepared.rs:190`) — also confirmed. `arrow-data` 59 `data.rs:510-561`
computes the same arms, and `GetSlicedSize` already wraps it for `RecordBatch`
in this crate (`spill/spill_manager.rs:222-240`), used in four places. Agreed
this should be reused.
One correction to the proposed substitution, though.
`batch.get_sliced_size()? + 64 * buffer_count` is **not** equivalent to the
current function on null handling:
```rust
// Arrow: validity counted only when the array actually has a null buffer
if self.nulls().is_some() {
result += bit_util::ceil(self.len, 8);
}
// PR: validity counted unconditionally, for every column
.and_then(|bytes| bytes.checked_add(rows.div_ceil(8)))
```
The PR is deliberately conservative here, and it has to be: `concat` of a
non-nullable-typed array with a nullable one materialises a validity buffer
that none of the inputs had. Measuring the inputs with Arrow's rule would then
under-admit the output. Any migration needs to keep the unconditional
`ceil(rows/8)` term (or add it back per column), not just swap in
`get_sliced_size`.
The current tests would not catch this: `plain_bytes.rs:36-37` makes both
byte columns nullable, and `tests.rs:46-47` is `Int64`, where the values term
dominates and the padding slack absorbs the difference. A mixed-nullability
fixture would be worth adding alongside the refactor.
**On the `prepared: bool` parameter** — three of us landed on this
independently, so I will not pile on further. @jayzhan211's sketch of pushing
the sizing into `prepared.rs` helpers and keeping one `if prepared` per charge
looks like the right shape, especially combined with @comphead's point that the
row-index term leaves entirely once it moves into `new_join_hashmap`.
**@comphead's equivalence-test gap** (`tests.rs:301`) is the one I most want
to endorse. No test currently asserts that a prepared build and an ordinary
`CollectLeft` build produce the same output over the same data — every test
checks a row count or a hand-written batch. That single property test is what
would catch a mis-shared `JoinLeftData`, and it is directly the claim the PR is
making. Same for running the plans under `multi_thread`; `tokio::join!` on the
default current-thread runtime does not exercise the concurrent-probe scenario
this exists for.
None of this changes my two design questions above — snapshot identity, and
whether the win is latency or memory. Those are still the ones I think need
answers before this lands.
--
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]
