Skip to content

fix: reject incomplete aggregate dynamic filters - #33

Merged
discord9 merged 3 commits into
greptimedb-53.1.0-function-signature-exec-errorfrom
fix/mixed-aggregate-dynamic-filter-53
Sep 10, 2026
Merged

discord9 merged 3 commits into
greptimedb-53.1.0-function-signature-exec-errorfrom
fix/mixed-aggregate-dynamic-filter-53

Conversation

@discord9

@discord9 discord9 commented Sep 10, 2026 •

Copy link
Copy Markdown

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 t can create a dynamic filter only for ts. Once a newer timestamp has been observed, this filter can prune older rows that would improve MAX(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?

  • Reject aggregate dynamic filtering if any MIN/MAX argument is not exactly one column reference.
  • Keep the existing OR-combined filter for supported direct-column MIN/MAX aggregates.
  • Replace the previous mixed-expression test that incorrectly expected subset filtering with regression coverage for both aggregate orders.
  • Correct the eligibility comments.
  • Adapt upstream fix: aggregate dynamic filtering with unsupported expressions apache/datafusion#24817’s two-file, two-row-group numerical regression: 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 -- --check passed 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.
  • Applying only the new regression test to its unpatched base fails because a dynamic filter is unexpectedly present.
  • A GreptimeDB HTTP reproduction with immutable data returned an incorrect maximum on one of 20 identical queries using the official 1.2.0 binary. A locally built GreptimeDB with this fix returned correct values in all 20 repetitions and no longer generated the incomplete filter.

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_regression passed with the adapted upstream regression. The same regression on the unpatched implementation fails numerically: expected 1 8 12 71, actual 1 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.

Signed-off-by: discord9 <discord9@163.com>
Signed-off-by: discord9 <discord9@163.com>
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>

@fengjiachun fengjiachun left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@discord9
discord9 merged commit bb531e7 into greptimedb-53.1.0-function-signature-exec-error Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants