fix(query): compare persisted statistics without collapsing to Equal - #20432
fix(query): compare persisted statistics without collapsing to Equal#20432youngsofun wants to merge 1 commit into
Conversation
Persisted min/max statistics are read back against the current schema but were written against the schema in effect at the time. Comparing two decimals of different DecimalSize has no defined ordering: DecimalScalar::partial_cmp returns None, which Ord for Scalar turns into Ordering::Equal and PartialEq turns into false. Neither raises an error, so a size mismatch silently degrades pruning, ordering, and overlap checks. Add stat_cmp with comparison helpers that key on scale and raw value, so decimals differing only in precision or storage variant compare correctly, and report None when values are genuinely incomparable. Migrate the statistics consumers to decide explicitly what an inconclusive comparison means for them instead of inheriting the silent Equal. reduce_column_statistics now returns Option: min_by/max_by previously took each end from a different input when comparisons collapsed, producing a range whose min and max disagreed on their type and tripping the assert in ColumnStatistics::new.
|
The
Required format (see AI_POLICY.md): The responsible human is the author-side owner — the person who has read the diff, |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b967b4fd2e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Some(Scalar::Decimal(match decimal { | ||
| DecimalScalar::Decimal64(value, _) => DecimalScalar::Decimal64(*value, target_size), | ||
| DecimalScalar::Decimal128(value, _) => DecimalScalar::Decimal128(*value, target_size), | ||
| DecimalScalar::Decimal256(value, _) => DecimalScalar::Decimal256(*value, target_size), |
There was a problem hiding this comment.
Normalize decimal storage variants when retagging bounds
When a precision widening crosses a backing-type threshold, such as DECIMAL(18,2) to DECIMAL(19,2), old bounds remain Decimal64 while new bounds are Decimal128. Preserving those variants while assigning both the target size lets reduce_column_statistics emit a Decimal64 minimum and Decimal128 maximum; statistics_to_domain then selects its decoder from the minimum and unwraps the same decoder for the maximum, causing range pruning to panic. Convert values to the target size's canonical storage variant, or make domain construction explicitly handle mixed variants.
Useful? React with 👍 / 👎.
| match self.values.binary_search_by(|entry| { | ||
| try_cmp_stat_scalars(&entry.scalar.as_ref(), scalar).unwrap_or(Ordering::Greater) | ||
| }) { |
There was a problem hiding this comment.
Use the statistics comparator when merging ColumnTopN
When segment TopN sketches from blocks written before and after a decimal precision widening are merged, ColumnTopN::merge still compares entries with lhs.scalar.cmp(&rhs.scalar). Unequal mixed-precision decimals therefore compare as Equal, so the merge consumes them as the same key and adds their counts; changing only find here does not protect normal segment-stat aggregation. Use the same statistics-aware ordering in the merge path and cover merging mixed-precision sketches.
Useful? React with 👍 / 👎.
I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/
Summary
Nonefor genuinely incomparable statistics, and make each consumer decide what that means instead of inheriting a silentOrdering::Equalreduce_column_statisticsproducing a range whose min and max disagree on their typeMotivation
Persisted statistics are read back against the current schema but were written against the schema in effect at the time. Comparing two decimals of different
DecimalSizehas no defined ordering —DecimalScalar::partial_cmpreturnsNone— and the surrounding impls absorb that:Ord for Scalarispartial_cmp(..).unwrap_or(Ordering::Equal), so<,>,cmp,min_by,max_by, andsort_bysilently report the values as equalPartialEq for Scalarispartial_cmp(..) == Some(Equal), so==silently reports them as differentNeither raises an error. A size mismatch therefore degrades pruning, read ordering, and overlap checks with no visible signal.
This is reachable today.
statistics_to_domainasserted the persisted size matched the schema size (range_index.rs), and statistics arriving from an external Parquet column index carry the size implied by the Parquet physical type rather than the schema. It also becomes reachable for fuse tables under any metadata-onlyMODIFY COLUMNthat widens a decimal precision without rewriting data.reduce_column_statisticsis the sharpest case. It pickedminandmaxwith separatemin_by/max_bypasses; when comparisons collapse toEqualeach pass takes its end from a different input, so the two ends can disagree on their type — which trips theassert!insideColumnStatistics::new.Implementation
New
meta/stat_cmp.rs:try_cmp_stat_scalars/try_cmp_stat_scalar_slices— compare by scale, then by raw value widened toi256. A decimal's raw value does not depend on its precision, so same-scale values are comparable whatever precision or variant they carry. ReturnOption<Ordering>.try_stat_ranges_disjoint— range disjointness,Nonewhen undecidableretag_stat_scalar/common_stat_decimal_size— reinterpret bounds at a target size for callers that must materialize a valuetotal_cmp_stat_scalars/total_cmp_stat_scalar_slices— a genuine total order forBTreeMapkeys, where returningEqualfor distinct values would merge map entriesBecause comparability depends only on scale, no consumer signature needed a
data_typeparameter;RuntimeTopNFilter::newand friends are unchanged.Each consumer picks its own failure direction:
reduce_column_statisticsNone, caller omits the columnstatistics_to_domainDomain::fulltopn_prunerRuntimeTopNFilter::boundary_excludescluster_stats_scalar_overlap,scalar_lereduce_cluster_min_maxsort_by_cluster_stats, rank comparison,to_partitionscheck_overlapped_by_stats,table_level_row_pruneColumnTopN::findGreater, keeping binary search terminating; the existing recheck rejects a bogus hitlinear_recluster::ScalarSlicereduce_column_statisticsnow aligns all bounds to the widest precision seen before comparing, so both ends agree on one size, and returnsOption.statistics_to_domainretags bounds to the schema size, which let the twodebug_assert_eq!onDecimalSizego away — they were asserting a property the data does not guarantee.Tests
19 new unit tests. Each was confirmed to fail without the fix by reverting the comparison helper and re-running, then confirmed to pass with it.
Notably, restoring the original
debug_assert_eq!inrange_index.rswhile disabling the retag reproduces the real panic:cargo test -p databend-storages-common-table-meta -p databend-storages-common-pruner -p databend-storages-common-index -p databend-common-catalog -p databend-common-storages-fuse --lib --testscargo test -p databend-query --test it parquet_rs(5 passed — these are the tests that fail if the comparison predicate wrongly rejects external-Parquet decimals)cargo test -p databend-query --test it storages::fuse::statistics(15 passed)cargo clippyon all five crates,--all-targets -- -D warningscargo fmt --all -- --checkTwo pre-existing failures, verified unrelated by reproducing them with this branch stashed on clean
main:storages::system::test_caches_tableandtest_columns_table(goldenfile drift, only when the fullstorages::suite runs). Thedatabend-common-storages-fusebench target needs/tmp/tpch_1/lineitem.parquetand fails on any checkout.Type of change
Notes for review
This is an alternative to #20418 and is meant to be chosen against it, not merged alongside. Both address the same hazard; the difference is scope.
This branch does not make
min/maxprivate and does not introduce a view type. It changes one signature (reduce_column_statisticsreturnsOption) and leaves the persisted structs and every other consumer signature alone. Whether that restraint is the right call is the main thing worth arguing about: #20418's private fields give compiler enforcement that comments cannot, at the cost of a wider diff. Themin/maxdoc comments here name the helpers, but nothing forces their use.Also worth flagging: the
PartialEqcollapse direction is the opposite ofOrd's, sovalidate_segment_partition_statisticswould report "different partitions" rather than degrade quietly. It is unreachable today becauseMODIFY COLUMNrejects any column referenced byPARTITION BY, and this PR leaves it untouched rather than adding speculative handling.AI assistance
This change is