Repository navigation
feat: re-land Decimal filter selectivity, and use the default selectivity for columns without known bounds - #27148
Open
shrivasshankar wants to merge 7 commits into
Open
shrivasshankar wants to merge 7 commits into
shrivasshankar wants to merge 7 commits into
Conversation
Re-applies apache#25790, which was reverted in apache#27145 because of a TPC-H Q18 regression (apache#27108). The next commit fixes the regression.
… stats Interval analysis estimates a selectivity of 1.0 for a column without bounds. In TPC-H Q18, `HAVING sum(l_quantity) > 300` filters an aggregate output, which has no min/max stats, so apache#25790 changed its estimate from the default 20% to 100% and the semi join built its hash table on the wrong side. Decimal predicates now use interval analysis only when every decimal column in them has min and max stats. Otherwise they keep the default selectivity, as before apache#25790.
github-actions
Bot
added
logical-expr
Contributor
Author
|
cc @asolimando @devanbenz @kosiew, this re-lands apache/datafusion#25790 · feat: estimate Decimal filter… with the Q18 fix. The Q18 plan check is in filter_statistics_decimal.slt. |
Interval analysis estimates an equality from the number of values in the column's range, and for a decimal that counts every step of its scale. In TPC-DS, `ca_gmt_offset = -5` on a DECIMAL(5, 2) column holding whole numbers from -10 to -5 was estimated at 100 rows instead of 18,246, which flipped the semi joins in q33, q56, q60, q61 and q91. Decimal equality filters now keep the estimate they had before apache#25790. Range filters on decimal columns still use interval analysis.
Contributor
Author
|
Update: while checking TPC-DS, I found that = filters on decimal columns were also badly underestimated (100 rows instead of 18,246 for ca_gmt_offset = -5), which flipped 5 TPC-DS plans. I added a commit so decimal equality keeps its old estimate. All TPC-H and TPC-DS plans now match main. |
xudong963
self-requested a review
November 6, 2026 14:05
Contributor
|
@kosiew @xudong963 Are you able to kick off the automated benchmarks? We should run all available benchmarks both with pushdown filtering on and off. If we can think of any other configurations which may be impacted by this as well please run those as well.
Better estimates is nice! But also, it would be cool to see metrics associated with performance goodness. If you have them please add them to the PR description :) |
Contributor
Author
Thanks! With this PR, every TPC-H (SF1, SF10) and TPC-DS (SF1) plan is identical to main, with pushdown_filters both on and off, so I wouldn’t expect runtime changes on those benchmarks. The gain for now is in the estimates themselves. For example, the Q6-style filter goes from 1.2M estimated rows to 111K (actual 114K). I’ll add the benchmark results to the description once they’ve run. |
asolimando
reviewed
Nov 9, 2026
asolimando
left a comment
asolimando
left a comment
Member
There was a problem hiding this comment.
Thanks @shrivasshankar for the re-land and for checking all TPC-H and TPC-DS plans! The comment I'd like to settle before merging is in statistics_helper: some decimal predicates that TPC-H and TPC-DS don't cover are now estimated at 100% instead of the 20% default, the same over-estimate that flipped Q18, so I consider this a blocking issue as much as the original problem that led to the revert.
| let null_rejecting_columns = collect_null_rejecting_columns(predicate); | ||
|
|
||
| if check_support(predicate, schema) { | ||
| if check_statistics_support(predicate, schema) |
Member
There was a problem hiding this comment.
On a 1100-row table (i INT in 0..99, w DECIMAL(38, 18) in 0..99, d DECIMAL(15, 2) in 0.00..0.10, reproducer below), these predicates are estimated at 1100 rows, while main gives the 20% default (220):
i < CAST(2.5 AS DECIMAL(5, 1)): 33 actualHAVING sum(i) > CAST(300.5 AS DECIMAL(10, 1)): 72 actual, the Q18 shapew < 10: 110 actualCAST(d AS DOUBLE) > 0.05: 500 actual
In all of them interval analysis can't narrow the column, so the selectivity is 1.0, which is worse than the default. The two new checks miss these causes:
- INT to decimal is lossless here, but
can_propagate_cast_constraintshas no rule for it, anddecimal_columns_have_boundsskips INT columns w's range has 99 × 10^18 values, more than theu64returned byInterval::cardinality, so the cardinality isNone- decimal to
Float64is lossy, so the bound is rightly not propagated, butcheck_statistics_supportaccepts the cast
HAVING sum(i) > 300 is also 1100 on main, so this is not decimal-specific. I wonder if cardinality_ratio should return None when a cardinality is unknown or propagation doesn't reach a column, so that statistics_helper falls back to the default for any type, and decimal_columns_have_bounds isn't needed (has_decimal_equality is a separate issue and should stay). WDYT?
Reproducer
# Reproducers for PR #27148. Paste at the end of filter_statistics_decimal.slt.
#
# t has 1100 rows: i cycles through 0..99 (INT), w through 0..99 as
# DECIMAL(38, 18), and d through 0.00..0.10 as DECIMAL(15, 2).
query I
COPY (
SELECT
CAST(value % 100 AS INT) AS i,
CAST(value % 100 AS DECIMAL(38, 18)) AS w,
CAST((value % 11) / 100.0 AS DECIMAL(15, 2)) AS d
FROM range(1100)
)
TO 'test_files/scratch/filter_statistics_decimal/t.parquet'
STORED AS PARQUET;
----
1100
statement ok
set datafusion.explain.physical_plan_only = true;
statement ok
set datafusion.explain.show_statistics = true;
statement ok
set datafusion.execution.target_partitions = 1;
statement ok
CREATE EXTERNAL TABLE t
STORED AS PARQUET
LOCATION 'test_files/scratch/filter_statistics_decimal/t.parquet';
# INT column compared with a decimal literal
query TT
EXPLAIN SELECT i FROM t WHERE i < CAST(2.5 AS DECIMAL(5, 1));
----
query I
SELECT count(*) FROM t WHERE i < CAST(2.5 AS DECIMAL(5, 1));
----
# Aggregate with INT output compared with a decimal literal (TPC-H Q18 shape)
query TT
EXPLAIN SELECT i FROM t GROUP BY i HAVING sum(i) > CAST(300.5 AS DECIMAL(10, 1));
----
query I
SELECT count(*) FROM (SELECT i FROM t GROUP BY i HAVING sum(i) > CAST(300.5 AS DECIMAL(10, 1)));
----
# Range filter on a DECIMAL(38, 18) column
query TT
EXPLAIN SELECT w FROM t WHERE w < 10;
----
query I
SELECT count(*) FROM t WHERE w < 10;
----
# Decimal column under a cast to DOUBLE
query TT
EXPLAIN SELECT d FROM t WHERE CAST(d AS DOUBLE) > 0.05;
----
query I
SELECT count(*) FROM t WHERE CAST(d AS DOUBLE) > 0.05;
----
# Aggregate with INT output compared with an INT literal: no decimal involved
query TT
EXPLAIN SELECT i FROM t GROUP BY i HAVING sum(i) > 300;
----
statement ok
DROP TABLE t;
statement ok
set datafusion.execution.target_partitions = 4;
statement ok
RESET datafusion.explain.physical_plan_only;
statement ok
RESET datafusion.explain.show_statistics;
Contributor
Author
There was a problem hiding this comment.
Thanks for the repro! Went with your suggestion: analyze now returns no selectivity when a column's cardinality is unknown, and FilterExec falls back to the default for any type, so decimal_columns_have_bounds is gone. (I did it in calculate_selectivity since cardinality_ratio is public.) Your four cases now estimate 33, 220, 220 and 220, and they're in the slt.
Since it's not decimal-only anymore, TPC-DS q8, q34, q70 and q73 change plans. Their count(*)/rank() filters were at 100% before. Details are in the description; all four ran the same or faster locally.
| // to strings) does not preserve the interval, so such expressions are | ||
| // left unanalyzed rather than producing bogus bounds. | ||
| is_datatype_supported(cast.cast_type()) && check_support(cast.expr(), schema) | ||
| is_type_supported(cast.cast_type()) |
Member
There was a problem hiding this comment.
CAST(d AS DOUBLE) > 0.05 is estimated at 1100 rows instead of 220 on main (500 actual, reproducer in my comment in statistics_helper). Decimal to Float64 and narrowing decimal casts are lossy, so the bound is not propagated back and the selectivity is 1.0.
I think this branch also requires can_propagate_cast_constraints(input_type, cast.cast_type()), can you double check?
Contributor
Author
There was a problem hiding this comment.
I added only in check_statistics_support though, since the SHJ sanity checker uses check_support.
| /// has at least as many fractional and integer digits as the source. Casting | ||
| /// bounds back to the source then rounds to a neighboring representable value, | ||
| /// which never excludes a valid input. | ||
| fn is_decimal_widening(source: &DataType, target: &DataType) -> bool { |
Member
There was a problem hiding this comment.
builder.rs has the same rule in recursive_type_can_be_coerced, which also handles INT to decimal (Int32 as (10, 0), Int64 as (19, 0)). Sharing one helper would let bounds propagate through CAST(int AS Decimal) too, fixing the i and sum(i) predicates from my comment in statistics_helper. wdyt?
Contributor
Author
There was a problem hiding this comment.
I moved it into a shared is_lossless_decimal_cast helper. i < 2.5 now estimates 33.
| .is_some_and(|field| field.data_type().is_decimal()); | ||
| !is_decimal | ||
| || column_statistics.get(column.index()).is_some_and(|stats| { | ||
| stats.min_value.get_value().is_some() |
Member
There was a problem hiding this comment.
Nit: a null min/max scalar (Precision::Exact(Decimal128(None, 15, 2))) passes this check, as get_value().is_some() is true, and gives a selectivity of 1.0. I couldn't reach it from SQL, but checking !v.is_null() would cover it. This test fails on the PR head (left: Inexact(1100), right: Inexact(220)):
#[tokio::test]
async fn test_filter_statistics_decimal_null_bounds() -> Result<()> {
// Table:
// a: Decimal128(15, 2), min=NULL, max=NULL
let schema =
Schema::new(vec![Field::new("a", DataType::Decimal128(15, 2), true)]);
let null_decimal = ScalarValue::Decimal128(None, 15, 2);
let input = Arc::new(StatisticsExec::new(
Statistics {
num_rows: Precision::Inexact(1100),
total_byte_size: Precision::Absent,
column_statistics: vec![ColumnStatistics {
min_value: Precision::Exact(null_decimal.clone()),
max_value: Precision::Exact(null_decimal),
..Default::default()
}],
},
schema.clone(),
));
// WHERE a > 0.03
let predicate = binary(
col("a", &schema)?,
Operator::Gt,
lit(ScalarValue::Decimal128(Some(3), 15, 2)),
&schema,
)?;
let filter: Arc<dyn ExecutionPlan> =
Arc::new(FilterExec::try_new(predicate, input)?);
let statistics =
StatisticsContext::new().compute(filter.as_ref(), &StatisticsArgs::new())?;
// Default selectivity (20%), as for a column without min/max
assert_eq!(statistics.num_rows, Precision::Inexact(220));
Ok(())
}
Contributor
Author
There was a problem hiding this comment.
covered by the cardinality change now. added your test as a case in test_filter_statistics_without_bounds.
| is_datatype_supported(data_type) | ||
| || matches!( | ||
| data_type, | ||
| DataType::Decimal32(_, _) |
Member
There was a problem hiding this comment.
Nit: data_type.is_decimal(), as in filter.rs, would be shorter and consistent.
…te a filter Interval analysis estimated a selectivity of 1.0 when it could not tell how many values of a column pass a predicate, which is worse than the default. This happened for columns without min/max, decimal ranges with more than u64::MAX values, and casts that bounds do not propagate back through. - `analyze` returns no selectivity when a column in the predicate has an unknown number of values, and `FilterExec` then uses the default selectivity, for any type. This replaces `decimal_columns_have_bounds`. - `check_statistics_support` rejects casts that bounds do not propagate back through, such as a decimal to Float64. - Integer to decimal casts that keep every value now propagate bounds. The check is shared with `builder.rs` as `is_lossless_decimal_cast`.
Member
|
Thanks @shrivasshankar, this addresses all my comments. As the fix now applies to every type, the title and description should say it's no longer decimal-only: filters on For the same reason, the automated benchmarks @devanbenz asked for matter even more now. The local SF1 runs look good, but I would wait for them before merging. |
shrivasshankar
changed the title
Contributor
Author
Thanks! Updated the title and description to make it clear it now applies to every type. Agreed, happy to wait for the benchmarks. |
Contributor
|
run benchmarks tpch tpch_mem tpcds |
Contributor
|
run benchmarks tpch tpcds env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: "true" |
jayzhan211
self-requested a review
November 14, 2026 09:17
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpch
env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: "true"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpcds
env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: "true"Results will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpch_memResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpch_memCPU Details (lscpu)Details
Resource Usagetpch_mem — base (merge-base)
tpch_mem — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpch
env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: "true"CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing reland-decimal-filter-selectivity (5b20d26) to 3b0d0c8 (merge-base) diff Run configurationrun benchmark tpcds
env:
DATAFUSION_EXECUTION_PARQUET_PUSHDOWN_FILTERS: "true"CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
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?
Rationale for this change
This PR does two things:
analysis can't estimate a filter, e.g. on a column without min/max such as
a
count(*)orrank()output. These filters were estimated at 100% ofrows before.
#25790 let interval analysis estimate decimal filters. When interval analysis
can't tell how many values of a column pass, it estimated 100% of rows, which
is worse than the 20% default. Q18's
HAVING sum(l_quantity) > 300hit this:the aggregate output has no min/max, and the bad estimate flipped the semi
join.
Equality on decimals also got bad estimates, since interval analysis counts
every scale step: TPC-DS
ca_gmt_offset = -5onDECIMAL(5, 2)was estimatedat 100 rows instead of 18,246.
What changes are included in this PR?
or a range too wide to count),
analyzereturns no selectivity andFilterExecuses the default. This applies to every type, so it alsochanges estimates on main, e.g.
HAVING count(*) > 10goes from 100% to20%.
DOUBLE)aren't analyzed. Integer to decimal casts that keep every value now
propagate bounds, using one helper shared with
builder.rs.What is the testing strategy for this PR?
Unit tests for each case, plus
filter_statistics_decimal.sltwith a Q18plan check and the cases from review.
All 22 TPC-H plans (SF1, SF10) match main, with
pushdown_filterson andoff.
TPC-DS (SF1): q8, q34, q70 and q73 change. They filter on
count(*)orrank()outputs, which have no min/max and were estimated at 100% on main.Locally they ran the same or faster:
The Q6-style query from Estimate Decimal filter selectivity #25613 still improves: 111,291 estimated rows
(actual 114,160) vs 1,200,243 on main.
Are there any user-facing changes?
Better estimates for range filters on decimal columns with min/max stats.
Filters on columns without min/max now use the default selectivity instead of
100%, for any type. One new public function each:
check_statistics_supportand
is_lossless_decimal_cast.