test: make push_down_filter_regression dynamic filter content deterministic (#22621)#22643
Open
diegoQuinas wants to merge 1 commit into
Open
test: make push_down_filter_regression dynamic filter content deterministic (#22621)#22643diegoQuinas wants to merge 1 commit into
diegoQuinas 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.