test: make push_down_filter_regression dynamic filter content deterministic (#22621)#22643
test: make push_down_filter_regression dynamic filter content deterministic (#22621)#22643diegoQuinas wants to merge 1 commit into
Conversation
…nistic The agg_dyn_* fixtures asserted the exact DynamicFilter content captured by EXPLAIN ANALYZE, but that content converges as parallel Partial aggregates publish their bounds. A snapshot can observe an intermediate value, making the test flaky (apache#22621). Give every file the same per-file min/max so any snapshot equals the converged filter, regardless of publish order. Applied to agg_dyn_single (the reported case) and to agg_dyn_two_col and agg_dyn_mixed, which shared the same latent race. Expected plan text is unchanged; only the input data and comments differ. Closes apache#22621
|
cc @xiedeyantu @Dandandan @kosiew Apparently this was introduced by #22150 |
@neilconway Thank you point it out, I will re-examine the original PR. |
|
@neilconway @diegoQuinas I reviewed my previous PR #22150 and couldn't find anything causing the problem. The current issue mentions instability due to concurrency; could the optimizations in the PR have caused changes in execution time that exposed this issue? If we can confirm that the filter is unstable, then the root cause might not be in my previous PR (because that test wasn't newly added in this PR). |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you -- this make sense to me @diegoQuinas
| # --- single-column fixture ([5, 1, 3, 8]) split across 2 files --- | ||
| # --- single-column fixture ([1, 8, 1, 8]) split across 2 files --- | ||
| # | ||
| # Every file shares the same per-file min (1) and max (8). This makes the |
| # Use `analyze_level = summary` + `analyze_categories = 'none'` so metrics | ||
| # render empty; we only care that the `predicate=DynamicFilter [ ... ]` text | ||
| # matches. Pruning metrics here are subject to a parallel-execution race | ||
| # matches. The pruning *counts* are still subject to a parallel-execution race |
There was a problem hiding this comment.
My concern with this approach is that it is changing the meaning of the test
However, it looks like we have had issues with this test before that @mbutrovich saw
So given that I agree the actual dynamic filter content is subject to a race condition this is best I can imagine us doing at this point
4 participants
Which issue does this PR close?
Rationale for this change
push_down_filter_regression.slt(added in #22150) asserts the exactDynamicFiltercontent rendered byEXPLAIN ANALYZEon theagg_dyn_*fixtures. That content is not deterministic: the filter threshold tightens
as each
AggregateExec(mode=Partial)publishes its runningmin/max, and theEXPLAIN ANALYZEsnapshot can be taken while the filter is still converging.For
agg_dyn_single,file_0holds the globalmin(1) andfile_1a largerpartial
min(3). If the snapshot lands afterfile_1publishes3but beforefile_0publishes1, the filter readsa < 3instead of the finala < 1—exactly the intermittent CI failure reported in #22621. The fixture's comment
incorrectly claimed the filter content was deterministic and only the pruning
counts raced.
What changes are included in this PR?
Make the filter content independent of publish order by giving every file the
same per-file min/max, so any snapshot equals the fully converged filter:
agg_dyn_single— both files(1), (8)→ each filemin=1, max=8.agg_dyn_two_col— each filemin(a)=1, max(b)=9.agg_dyn_mixed— each filemin(a)=1, max(a)=8, max(b)=12.agg_dyn_two_colandagg_dyn_mixedwere not in the reported failure but sharedthe same latent race (differing per-file extremes), so they are fixed too.
agg_dyn_nullsis left untouched — its filter is alwaystrueand never races.The expected plan text is unchanged; only the input data and the misleading
comments are modified. The alternative of forcing a single partition was
rejected: dynamic aggregate filters are only emitted in
Partial+Finalmode(
target_partitions >= 2), so a single partition would emit no filter at all.Are these changes tested?
Yes — the modified
push_down_filter_regression.sltitself is the test. Itpasses, and because the asserted filter content no longer depends on partition
scheduling, it is stable across runs (verified by running it repeatedly locally).
Are there any user-facing changes?
No. Test-only change.