Conversation
Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <discord9@163.com>
4 of 6 tasks
discord9
marked this pull request as ready for review
September 10, 2026 10:54
Adapted from apache#24817, commit b3cb365. Retain the equivalent 53.1.0 eligibility guard without unrelated metadata renames. Signed-off-by: discord9 <discord9@163.com>
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
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?
Upstream fix: apache#24817 (merged as
b3cb365dd6e5e0441f3f58817fee55dae1814fdb).GreptimeDB dependency update: GreptimeTeam/greptimedb#9102
Rationale for this change
SELECT MAX(ABS(v)), MAX(ts) FROM tcan create a dynamic filter only forts. Once a newer timestamp has been observed, this filter can prune older rows that would improveMAX(ABS(v)). The two aggregates share input, so deriving a filter from only a subset is unsafe. Results can depend on scan timing.What changes are included in this PR?
MIN(c + 1)must be 71, with no incomplete filter.This targets the 53.1.0 fork branch currently used by GreptimeDB main. Apache DataFusion apache#24817 has already fixed the same issue on upstream main. The current patch is an independently implemented equivalent of its correctness guard, not a direct cherry-pick. A follow-up commit adapts the upstream deterministic numeric Parquet regression to 53.1.0, with provenance recorded in the commit message. No additional upstream PR is needed. The published DataFusion 55.0.0 source still contains the defect.
Are these changes tested?
cargo fmt --all -- --checkpassed on the original fix checkpoint.cargo test -p datafusion --test core_integration aggregate_dynamic_filter -- --nocapture: 9 passed on the original 53.1.0 fork checkpoint.The patch has been cherry-picked onto the current 53.1.0 fork dependency branch. The same 9 aggregate dynamic-filter tests also pass on the exact published head
127df0938f8476d15e52ea9402fc6064adec098b. This PR remains a draft pending CI/review.Additional verification:
cargo test -p datafusion-sqllogictest --test sqllogictests -- push_down_filter_regressionpassed with the adapted upstream regression. The same regression on the unpatched implementation fails numerically: expected1 8 12 71, actual1 8 12 101; it also exposes the unwanted dynamic filter. The patched test passes.Are there any user-facing changes?
Correct results for mixed expression/column MIN/MAX queries. Such mixed aggregates no longer use this unsafe scan optimization. No public API or persisted-format changes.