rgbuilds commented on PR #26123:
URL: https://github.com/apache/datafusion/pull/26123#issuecomment-6115011186
> @rgbuilds ah yeah missed that, both sides were 123 bytes so the swap never
triggered. forced the build side bigger with a registry now and added ptr_eq
checks on both children, should actually exercise the guard now
Thanks—the build-side statistics override and the `Arc::ptr_eq` assertions
are useful improvements. However, I repeated the guard-removal check locally at
commit `1643c176`, and the regression test still passes when the statistical
dynamic-filter guard is disabled.
For the ablation, I temporarily changed:
```rust
if !hash_join.dynamic_expressions_produced().is_empty() {
```
to:
```rust
if false && !hash_join.dynamic_expressions_produced().is_empty() {
```
I then reran:
```bash
cargo test -p datafusion \
--test core_integration \
test_join_selection_skips_real_dynamic_filter_pushdown \
-- --nocapture
```
The test still passed.
The build-side statistics provider is being applied, but after
`FilterPushdown` wires the dynamic filter into the probe side, the statistics
visible to `should_swap_join_order` are:
```text
left/build:
num_rows = Inexact(262144)
total_byte_size = Inexact(8388608)
right/probe:
num_rows = Absent
total_byte_size = Absent
```
`should_swap_join_order` first compares `total_byte_size` when both sides
have values. If that is not possible, it compares `num_rows` when both sides
have values. Because the probe side has neither value, both comparisons are
unavailable and the function returns `false`.
Therefore, with the guard removed:
1. `JoinSelection` still does not call `swap_inputs`.
2. The left and right children remain unchanged.
3. The `Arc::ptr_eq` assertions pass.
4. The test succeeds even though the guard is absent.
Could we also register small statistics for plans whose schema contains the
`probe_` fields? For example:
```rust
registry.register(Arc::new(ClosureStatisticsProvider::with_matches(
|plan| {
plan.schema()
.fields()
.iter()
.any(|f| f.name().starts_with("probe_"))
},
|_plan, _child_stats| {
Ok(StatisticsResult::Computed(small_statistics().into()))
},
)));
```
I tested this locally. With the additional probe-side provider, the
statistics become comparable:
```text
build total_byte_size = 8,388,608
probe total_byte_size = 32,768
```
With those statistics and the guard disabled, `should_swap_join_order`
returns `true`, `JoinSelection` reaches `swap_inputs`, and the test fails with
the expected error:
```text
Cannot swap HashJoinExec inputs after dynamic filters have been constructed
```
That gives the regression test the behavior we want:
```text
guard present
→ dynamically filtered join is skipped
→ original children are preserved
→ execution succeeds
guard removed
→ statistics request a swap
→ swap_inputs rejects the dynamically filtered join
→ test fails
```
--
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]
