Re: [PR] Per-conjunct pruning statistics for PruningPredicate [datafusion]

2026-07-20 Thread via GitHub 


github-actions[bot] commented on PR #22235:
URL: https://github.com/apache/datafusion/pull/22235#issuecomment-5029451246

   Thank you for your contribution. Unfortunately, this pull request is stale 
because it has been open 60 days with no activity. Please remove the stale 
label or comment or this will be closed in 7 days.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Per-conjunct pruning statistics for PruningPredicate [datafusion]

2026-05-21 Thread via GitHub 


asolimando commented on code in PR #22235:
URL: https://github.com/apache/datafusion/pull/22235#discussion_r3280521346


##
datafusion/datasource-parquet/src/page_filter.rs:
##
@@ -336,6 +410,190 @@ impl PagePruningAccessPlanFilter {
 pub fn filter_number(&self) -> usize {
 self.predicates.len()
 }
+
+/// Like [`Self::prune_plan_with_page_index`] but also surfaces, as a
+/// side-effect of the pruning iteration that already runs, a
+/// per-conjunct accumulator with the rows that conjunct alone
+/// would have proven skippable. Callers use this to seed a
+/// per-FilterId selectivity prior without doing any extra pruning
+/// work — every page-index lookup that would have happened in
+/// `prune_plan_with_page_index` happens exactly once here too.
+pub fn prune_plan_with_per_conjunct_stats(
+&self,
+mut access_plan: ParquetAccessPlan,
+arrow_schema: &Schema,
+parquet_schema: &SchemaDescriptor,
+parquet_metadata: &ParquetMetaData,
+file_metrics: &ParquetFileMetrics,
+) -> (ParquetAccessPlan, Vec, usize) {
+// scoped timer updates on drop
+let _timer_guard = file_metrics.page_index_eval_time.timer();
+
+let mut per_conjunct: Vec = 
(0..self.predicates.len())
+.map(|i| PerConjunctPageStats {
+tag: self.tags.as_ref().and_then(|t| t.get(i).copied()),
+rows_seen: 0,
+rows_skipped: 0,
+})
+.collect();
+
+if self.predicates.is_empty() {
+return (access_plan, per_conjunct, 0);
+}
+
+let groups = parquet_metadata.row_groups();
+if groups.is_empty() {
+return (access_plan, per_conjunct, 0);
+}
+
+if parquet_metadata.offset_index().is_none()
+|| parquet_metadata.column_index().is_none()
+{
+return (access_plan, per_conjunct, 0);
+}
+
+// Same accumulators as the untagged path, plus per-conjunct.

Review Comment:
   The comment shows that you are aware of the duplication w.r.t. the untagged 
path, is this a temporary state for the draft? (with draft PRs it's sometimes a 
bit tricky to tell what is what, apologies if that should be evident at this 
point)



##
datafusion/pruning/src/pruning_predicate.rs:
##
@@ -568,6 +697,50 @@ impl PruningPredicate {
 Ok(builder.build())
 }
 
+/// Like [`Self::prune`] but also returns per-conjunct pruning
+/// stats when this predicate was constructed via
+/// [`Self::try_new_tagged_conjuncts`]. The `Vec` is the same
+/// AND'd result `prune` would return; the per-conjunct stats are
+/// computed by evaluating each leaf sub-predicate against the same
+/// `statistics` and counting pruned/seen containers.
+///
+/// Returns `(prune_result, vec![])` for predicates constructed via
+/// the plain [`Self::try_new`] (no sub-predicates available).
+pub fn prune_per_conjunct(
+&self,
+statistics: &S,
+) -> Result<(Vec, Vec)> {
+let combined = self.prune(statistics)?;
+let Some(sub_predicates) = &self.sub_predicates else {
+return Ok((combined, Vec::new()));
+};
+
+let total_containers = statistics.num_containers();
+let mut per_conjunct: Vec =
+Vec::with_capacity(sub_predicates.len());
+for sub in sub_predicates {
+let kept = sub.predicate.prune(statistics)?;

Review Comment:
   It seems that we are looping through the predicates twice, the second time 
here, but IIUC we already do a full traversal at line 713, I wonder if we could 
do both at the same time as this might be costly with many conjuncts/deep 
expressions?



##
datafusion/pruning/src/pruning_predicate.rs:
##
@@ -499,9 +544,77 @@ impl PruningPredicate {
 required_columns,
 orig_expr: expr,
 literal_guarantees,
+sub_predicates: None,
 })
 }
 
+/// Like [`Self::try_new`] but takes already-split top-level
+/// conjuncts each carrying a caller tag (typically a `FilterId`).
+/// Builds one [`PruningPredicate`] per conjunct as a leaf
+/// sub-predicate. The wrapper itself is a marker holding the
+/// leaves; calls to [`Self::prune`] AND the leaves' results,
+/// avoiding a redundant combined-predicate construction.
+/// [`Self::prune_per_conjunct`] also reads from the same leaves.
+///
+/// Conjuncts whose individual `try_new` would error or return
+/// always-true are silently skipped (their tags do not appear in
+/// the per-conjunct output).
+pub fn try_new_tagged_conjuncts(
+tagged: &[(usize, Arc)],
+schema: SchemaRef,
+) -> Result {
+// Build per-conjunct PruningPredicates (each is a leaf — i.e.
+// its own `sub_predicates` is `None`).
+let mut sub_predicates

Re: [PR] Per-conjunct pruning statistics for PruningPredicate [datafusion]

2026-05-16 Thread via GitHub 


asolimando commented on PR #22235:
URL: https://github.com/apache/datafusion/pull/22235#issuecomment-4466312804

   > If you're willing to help review / get this in good shape for a review 
from a committer I think that would go a long way towards getting it across the 
line. I've mostly gotten it to work without paying too much attention to the 
API, interaction with existing methods, etc., hence why this is in draft.
   
   Sure, I will put it on my to-do list for next week when I am back from 
holidays!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Per-conjunct pruning statistics for PruningPredicate [datafusion]

2026-05-15 Thread via GitHub 


adriangb commented on PR #22235:
URL: https://github.com/apache/datafusion/pull/22235#issuecomment-4466057458

   If you're willing to help review / get this in good shape for a review from 
a committer I think that would go a long way towards getting it across the 
line. I've mostly gotten it to work without paying too much attention to the 
API, interaction with existing methods, etc., hence why this is in draft.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Per-conjunct pruning statistics for PruningPredicate [datafusion]

2026-05-15 Thread via GitHub 


asolimando commented on PR #22235:
URL: https://github.com/apache/datafusion/pull/22235#issuecomment-4465657140

   This would also be extremely useful for bootstrapping a cold stats in 
catalog via execution and runtime stats, I hope this feature lands!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Re: [PR] Per-conjunct pruning statistics for PruningPredicate [datafusion]

2026-05-15 Thread via GitHub 


github-actions[bot] commented on PR #22235:
URL: https://github.com/apache/datafusion/pull/22235#issuecomment-4464067698

   
   Thank you for opening this pull request!
   
   Reviewer note: 
[cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) 
reported the current version number is not SemVer-compatible with the changes 
in this pull request (compared against the base branch).
   
   
   Details
   
   ```
Cloning apache/main
   Building datafusion-datasource-parquet v53.1.0 (current)
  Built [  41.981s] (current)
Parsing datafusion-datasource-parquet v53.1.0 (current)
 Parsed [   0.029s] (current)
   Building datafusion-datasource-parquet v53.1.0 (baseline)
  Built [  41.559s] (baseline)
Parsing datafusion-datasource-parquet v53.1.0 (baseline)
 Parsed [   0.028s] (baseline)
   Checking datafusion-datasource-parquet v53.1.0 -> v53.1.0 (no change; 
assume patch)
Checked [   0.189s] 222 checks: 222 pass, 30 skip
Summary no semver update required
   Finished [  85.520s] datafusion-datasource-parquet
   Building datafusion-physical-expr-common v53.1.0 (current)
  Built [  19.150s] (current)
Parsing datafusion-physical-expr-common v53.1.0 (current)
 Parsed [   0.022s] (current)
   Building datafusion-physical-expr-common v53.1.0 (baseline)
  Built [  18.963s] (baseline)
Parsing datafusion-physical-expr-common v53.1.0 (baseline)
 Parsed [   0.022s] (baseline)
   Checking datafusion-physical-expr-common v53.1.0 -> v53.1.0 (no change; 
assume patch)
Checked [   0.266s] 222 checks: 222 pass, 30 skip
Summary no semver update required
   Finished [  39.577s] datafusion-physical-expr-common
   Building datafusion-proto v53.1.0 (current)
  Built [  52.530s] (current)
Parsing datafusion-proto v53.1.0 (current)
 Parsed [   0.142s] (current)
   Building datafusion-proto v53.1.0 (baseline)
  Built [  53.624s] (baseline)
Parsing datafusion-proto v53.1.0 (baseline)
 Parsed [   0.147s] (baseline)
   Checking datafusion-proto v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [   2.254s] 222 checks: 221 pass, 1 fail, 0 warn, 30 skip
   
   --- failure enum_variant_added: enum variant added on exhaustive enum ---
   
   Description:
   A publicly-visible enum without #[non_exhaustive] has a new variant.
   ref: 
https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
  impl: 
https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.47.0/src/lints/enum_variant_added.ron
   
   Failed in:
 variant ExprType:OptionalFilter in 
/home/runner/work/datafusion/datafusion/datafusion/proto/src/generated/prost.rs:1397
 variant ExprType:OptionalFilter in 
/home/runner/work/datafusion/datafusion/datafusion/proto/src/generated/prost.rs:1397
   
Summary semver requires new major version: 1 major and 0 minor checks 
failed
   Finished [ 111.070s] datafusion-proto
   Building datafusion-pruning v53.1.0 (current)
  Built [  36.108s] (current)
Parsing datafusion-pruning v53.1.0 (current)
 Parsed [   0.013s] (current)
   Building datafusion-pruning v53.1.0 (baseline)
  Built [  36.494s] (baseline)
Parsing datafusion-pruning v53.1.0 (baseline)
 Parsed [   0.013s] (baseline)
   Checking datafusion-pruning v53.1.0 -> v53.1.0 (no change; assume patch)
Checked [   0.102s] 222 checks: 222 pass, 30 skip
Summary no semver update required
   Finished [  74.652s] datafusion-pruning
   ```
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]