Skip to content

Per-conjunct pruning stats via PruningPredicateTree + observer#22498

Draft
adriangb wants to merge 2 commits into
apache:mainfrom
pydantic:pruning-predicate-tree-observer
Draft

Per-conjunct pruning stats via PruningPredicateTree + observer#22498
adriangb wants to merge 2 commits into
apache:mainfrom
pydantic:pruning-predicate-tree-observer

Conversation

@adriangb
Copy link
Copy Markdown
Contributor

@adriangb adriangb commented May 24, 2026

Which issue does this PR close?

Rationale for this change

#22235 surfaced per-conjunct pruning effectiveness by bolting a second, AND-only code path onto PruningPredicate (a marker wrapper holding sub_predicates, a placeholder true literal as its predicate_expr, and a prune_per_conjunct that re-walks the leaves a second time). Review feedback (thanks @asolimando) flagged three things, all tracing back to that shape:

  1. Double traversal — prune walks the leaves, then prune_per_conjunct walks them again.
  2. Duplicated page-pruning method (prune_plan_with_per_conjunct_stats) that is a near-copy of the untagged path.
  3. The wrapper's predicate_expr() / literal_guarantees() / required_columns() silently return placeholder values, which is misuse-prone.

Plus two design questions: AND-only (OR/NOT?), and order-dependent stats from the page-index short-circuit (later conjuncts get biased rows_seen).

This PR reshapes the feature around a small, single-pass, observer-driven primitive that resolves all of the above.

What changes are included in this PR?

Two reviewable commits, each builds green:

  1. PruningConjunction (datafusion-pruning) — an AND of tagged PruningPredicate leaves. One evaluation pass ANDs the leaves' masks and fires PruningObserver::on_leaf(tag, mask) only for leaves actually evaluated; the AND short-circuits once everything is pruned, so stats are never biased by predicates that didn't run. Built with a small builder:

    let conj = PruningConjunction::builder(schema)
        .push(filter_id, expr)   // runs try_new, skips always-true / errors
        .build();                // Option<PruningConjunction>

    PruningConjunction::single(predicate) wraps one existing predicate for the untagged path (prune() + NoopObserver, zero overhead). ConjunctStatsObserver accumulates PerConjunctPruneStats { tag, containers_seen, containers_pruned }.

    Deliberately AND-only. OR / NOT are handled inside a leaf via PruningPredicate::try_new, not as structural nodes, because (a) try_new(a < 5 OR b > 100) prunes better than ORing two leaf masks, and (b) the per-leaf "containers pruned" count is only sound under monotonic AND — under OR a leaf only helps where every sibling also prunes, so a per-leaf stat would mislead a scheduler. Per-branch OR short-circuit, where it is genuinely useful, belongs to the row-filter decode path over exact masks — a separate concern that should land with its consumer (the adaptive scheduler), not here.

  2. Parquet row-group + page filters consume the conjunction via *_with_observer methods. The page-index loop has one shared body (no duplicated method); its !selects_any short-circuit fires after a leaf was observed, so unevaluated leaves are correctly absent from stats.

How this maps to the review on #22235

Concern Resolution
Double traversal Single observer-driven pass
Duplicated page method One shared body, observer param
Placeholder predicate_expr Gone — leaves are real, standalone PruningPredicates
AND-only (Q1) Intentional: OR/NOT fold into leaves (prune better, sound stats). Exact-mask OR short-circuit is a decode-path concern, deferred.
Order-dependent stats (Q2) Short-circuited leaves are never observed

Are these changes tested?

Yes — unit tests for PruningConjunction (single-leaf equivalence to a plain PruningPredicate, AND short-circuit leaving later-leaf stats unbiased, builder stat accumulation, always-true dropping, all-trivial → None) and a row-group filter test asserting the observer surfaces correct per-conjunct stats while preserving the existing pruning result. All existing datafusion-datasource-parquet lib tests pass unchanged.

Are there any user-facing changes?

New public API in datafusion-pruning (PruningConjunction, PruningConjunctionBuilder, PruningObserver, NoopObserver, ConjunctStatsObserver, PerConjunctPruneStats, Tag) and an observer-accepting prune method on the parquet row-group / page filters. Purely additive; existing prune / prune_by_statistics / prune_plan_with_page_index paths are unchanged.

🤖 Generated with Claude Code

@github-actions github-actions Bot added the datasource Changes to the datasource crate label May 24, 2026
@adriangb adriangb force-pushed the pruning-predicate-tree-observer branch 4 times, most recently from d2f7474 to 7269c5b Compare May 24, 2026 15:28
Adds `PruningConjunction` — an AND of tagged `PruningPredicate` leaves —
plus a `PruningObserver` trait fired once per leaf actually evaluated.
The AND short-circuits once every container is pruned; short-circuited
leaves are not observed, so per-conjunct stats are never biased by
predicates that did not run.

Construction is via a small builder:

    let conj = PruningConjunction::builder(schema)
        .push(filter_id, expr)   // runs try_new, skips always-true / errors
        .build();                // Option<PruningConjunction>

`PruningConjunction::single(predicate)` wraps one already-built
predicate for the untagged path, which uses `prune()` (a `NoopObserver`)
at zero overhead. `combined_orig_expr()` exposes the AND of the leaves'
original expressions so consumers can invert the whole predicate (e.g.
for fully-matched detection) without re-deriving it.

`ConjunctStatsObserver` accumulates `PerConjunctPruneStats { tag,
containers_seen, containers_pruned }`. The observer is plumbed as
`&mut dyn PruningObserver` so consumers can hold it behind an `Option`.

OR and NOT are intentionally *not* structural nodes: they are handled
inside a leaf via `PruningPredicate::try_new`, which both prunes better
(cross-branch reasoning) and keeps the per-leaf "containers pruned"
count sound (it is only monotonic under AND). Per-branch OR
short-circuit, where it is useful, belongs to the row-filter decode
path over exact masks — not here.

Replaces the placeholder-`true`-literal `sub_predicates` design and its
double traversal from the earlier draft (apache#22235).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@adriangb adriangb force-pushed the pruning-predicate-tree-observer branch from 7269c5b to 73036f9 Compare May 24, 2026 15:53
Both filters expose a fluent, observer-capable prune op alongside the
existing methods (kept as thin shims, deprecatable later):

    filter.prune_row_groups(&ctx, &conjunction)
        .with_observer(&mut obs)   // optional
        .prune();

    let result = filter.prune_pages(access_plan, &ctx)
        .with_observer(&mut obs)   // optional
        .prune();

The four always-together refs (arrow/parquet schema, metadata/groups,
metrics) are bundled into `RowGroupPruningContext` / `PagePruningContext`
so no method carries a long argument list. The observer-accepting ops
are `pub(crate)` — the consumer (adaptive parquet scan) is in-crate — so
the public API stays `prune_by_statistics` / `prune_plan_with_page_index`,
unchanged.

`RowGroupAccessPlanFilter`: `prune_by_statistics` now wraps the predicate
via `PruningConjunction::single` and delegates; fully-matched detection
derives the combined expression from the conjunction's
`combined_orig_expr()` rather than taking a separate predicate argument.

`PagePruningAccessPlanFilter`: predicates carry an optional `Tag`
internally (`TaggedPagePredicate`); a builder replaces the old
tuple-slice `new_tagged`. The page-index loop emits one observer event
per leaf actually evaluated — leaves cut off by the `!selects_any`
short-circuit are not observed (resolves reviewer Q2 on apache#22235: stats are
not biased by predicates that never ran). The untagged and observer paths
share one body (`run_prune`); no duplicated method.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@adriangb adriangb force-pushed the pruning-predicate-tree-observer branch from 73036f9 to 897f9ea Compare May 24, 2026 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

datasource Changes to the datasource crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant