Repository navigation
feat: estimate Decimal filter selectivity with interval analysis - #25790
shrivasshankar wants to merge 6 commits into
Conversation
Decimal columns were excluded from interval analysis, so any FilterExec predicate touching a decimal column fell back to the default selectivity. - Add `check_statistics_support`, which also accepts Decimal32/64/128/256, and use it for FilterExec statistics. `check_support` and `is_datatype_supported` are unchanged, so symmetric hash join pruning keeps its current behavior. - Step decimals by one unit in the last place in `next_value`/`prev_value` so strict comparisons tighten bounds, saturating to unbounded at the extremes of the decimal's precision. - Support Decimal32/64 in `get_extreme_value!` for overflow handling. - Allow reverse constraint propagation through lossless decimal widening casts, so predicates whose literal has a different scale than the column (e.g. `CAST(a AS Decimal128(30, 15)) >= 0.055`) still narrow the column.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25790 +/- ##
========================================
Coverage 82.94% 82.94%
========================================
Files 1156 1156
Lines 458581 458871 +290
Branches 458581 458871 +290
========================================
+ Hits 380382 380630 +248
- Misses 55145 55162 +17
- Partials 23054 23079 +25 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
hi @shrivasshankar |
… decimal-filter-selectivity
Thanks @kosiew! Conflicts are resolved now, ready for review. |
kosiew
left a comment
There was a problem hiding this comment.
Thanks for working on decimal filter selectivity. The approach is appropriate, and the focused tests cover important decimal boundaries and cast behavior.
Keeping decimal support specific to statistics estimation, rather than expanding the existing symmetric hash join eligibility gate, is the right architectural boundary. The shared expression traversal also avoids unnecessary duplication. However, enabling decimal types must not admit arithmetic propagation that violates source column type invariants.
I'm requesting changes because valid decimal arithmetic predicates can now fail filter construction or produce column statistics with incorrect decimal types. These are correctness regressions that need to be addressed before merging. The additional SQL-level and cast-boundary tests are useful but non-blocking suggestions.
| ) -> bool { | ||
| check_support_with(expr, schema, |data_type| { | ||
| is_datatype_supported(data_type) | ||
| || matches!( |
There was a problem hiding this comment.
Decimal arithmetic can propagate bounds with the wrong source type. For a: Decimal128(15,2) with statistics [1.00,10.00], a * Decimal128(2.00,15,2) > Decimal128(5.0000,31,4) makes FilterExec::try_new fail with Selectivity is out of limit: 8324028856.826859 because inverse propagation retains scale 4; a - 2.00 > 5.00 also publishes Decimal128(17,2) bounds for a Decimal128(15,2) column. Please restore source-typed bounds with conservative outward rounding, or exclude decimal arithmetic from statistics eligibility. Clamping selectivity alone is insufficient. Add regressions for multiplication construction/selectivity and subtraction min/max types.
| let schema = | ||
| Schema::new(vec![Field::new("a", DataType::Decimal128(15, 2), false)]); | ||
| let input = Arc::new(StatisticsExec::new( | ||
| Statistics { |
There was a problem hiding this comment.
Optional follow-up: consider a SQLLogicTest for a Q6-style decimal range with ordinary fractional literals and known min/max statistics, asserting EXPLAIN estimates and query results. The current mixed-scale test constructs the cast explicitly, so it does not cover SQL coercion through physical expression construction to statistics estimation.
| Ok(()) | ||
| } | ||
|
|
||
| #[test] |
There was a problem hiding this comment.
Optional follow-up: consider representative containment tests for negative scales, cross-width widening, and bounds near precision limits. The exhaustive test covers only Decimal128 scale 2 to 3 near zero, while the Decimal32 to Decimal128 case currently checks eligibility rather than boundary propagation.
Decimal arithmetic changes the precision and scale of its result, and propagating constraints back to its operands did not restore their types. This could make `FilterExec::try_new` fail (e.g. `a * 2.00 > 5.0000`) or publish column bounds with the wrong decimal type (e.g. `a - 2.00 > 5.00`). `check_statistics_support` now only accepts decimals outside arithmetic; operands of `+ - * / %` must pass `is_datatype_supported`, so decimal arithmetic predicates fall back to the default selectivity. Also adds: - regression tests for decimal multiplication and subtraction predicates - `filter_statistics_decimal.slt`, covering Q6-style decimal ranges with fractional literals through SQL coercion, EXPLAIN estimates and results - decimal widening boundary tests for cross-width casts, negative scales, and bounds near the precision limits
Thanks @kosiew, good catches! I went with excluding decimal arithmetic from statistics estimation, so predicates like Added tests for your multiplication and subtraction cases, plus both of your suggestions: a sqllogictest with Q6-style fractional literals on a parquet table, and more cast boundary cases (cross-width, negative scale, near the precision limits). |
kosiew
left a comment
There was a problem hiding this comment.
Thanks for the follow-up changes. The Decimal filter selectivity improvements now have appropriate correctness safeguards and regression coverage. The approach looks good.
Keeping Decimal arithmetic outside interval-based statistics estimation is the right architectural boundary for now. Arithmetic can change precision and scale, so propagating constraints without accounting for those type changes could produce incorrect column bounds. Retaining the default selectivity for unsupported expressions while allowing safe Decimal comparisons, widening casts, and negation is a sound approach.
The previous correctness blocker is addressed, and the additional tests cover the requested arithmetic fallback, mixed-scale coercion, and end-to-end SQL behavior. I found no remaining actionable issues. Approved.
Thanks for the review @kosiew! Updated the branch with main. |
Which issue does this PR close?
Rationale for this change
Decimal columns were excluded from interval analysis, so any filter touching
one fell back to the default 20% selectivity. For the Q6-style predicate in the
issue, that meant estimating ~1.2M rows when only ~114K match.
What changes are included in this PR?
FilterExecstatistics use a newcheck_statistics_support, which alsoaccepts Decimal32/64/128/256.
next_value/prev_valuehandle decimals, so strict comparisons get tightbounds (
x < 24.00becomesx <= 23.99). At the limit of a decimal'sprecision the bound becomes unbounded instead of overflowing.
get_extreme_value!supports Decimal32/64, which previously hitunreachable!().CastExprnow propagates constraints back through lossless decimal wideningcasts. Without this, a literal with a different scale than the column (e.g.
a >= 0.055onDecimal(15, 2), coerced toDecimal128(30, 15)) left thecolumn range untouched and the estimate at 100%.
+ - * / %) is not analyzed: it changes the precisionand scale of its operands, so those predicates keep the default selectivity.
check_supportandis_datatype_supportedare unchanged, so symmetric hashjoin pruning behaves as before. Decimal support there could be a follow-up.
What is the testing strategy for this PR?
precision boundaries like 9.99 / -9.99 for
Decimal(3, 2), plus strictcomparison bounds and Decimal32/64 overflow handling.
test_filter_statistics_decimal_exprandtest_filter_statistics_decimal_mixed_scale_exprcheck row estimates andtightened min/max for same-scale and mixed-scale predicates. The mixed-scale
test mirrors the plan the SQL planner produces.
test_filter_statistics_decimal_arithmetic_exprchecks that decimalmultiplication and subtraction predicates use the default selectivity and
keep the column's own type.
filter_statistics_decimal.sltruns Q6-style ranges with fractionalliterals on a parquet table with min/max stats, checking EXPLAIN estimates
and query results, including a mixed-scale literal and decimal arithmetic.
test_cast_constraint_propagation, plus boundary checks thatpropagating through a decimal widening cast never excludes a valid input,
covering cross-width casts, a negative scale and the precision limits.
test_custom_filter_selectivityrelied on decimals being unsupported, so Iswitched it to
Utf8.Reproducer from the issue (TPC-H SF1, current main):
Worth a closer look in review:
lossless and order-preserving, and casting bounds back rounds to a
neighboring value, which only widens the range. That reasoning is what
makes it safe, so I'd appreciate a second opinion on it.
the default, which matches how integer columns already behave.
Are there any user-facing changes?
Better row estimates for filters on decimal columns, which may change some
plan choices. One new public function,
check_statistics_support; no breakingchanges.