Skip to content

fix(query): compare persisted statistics without collapsing to Equal - #20432

Draft
youngsofun wants to merge 1 commit into
databendlabs:mainfrom
youngsofun:codex/stat-comparator
Draft

fix(query): compare persisted statistics without collapsing to Equal#20432
youngsofun wants to merge 1 commit into
databendlabs:mainfrom
youngsofun:codex/stat-comparator

Conversation

@youngsofun

@youngsofun youngsofun commented Sep 2, 2026

Copy link
Copy Markdown
Member

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

  • Compare persisted min/max statistics by scale and raw value, so decimals differing only in precision or storage variant order correctly
  • Report None for genuinely incomparable statistics, and make each consumer decide what that means instead of inheriting a silent Ordering::Equal
  • Fix reduce_column_statistics producing a range whose min and max disagree on their type

Motivation

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 DecimalSize has no defined ordering — DecimalScalar::partial_cmp returns None — and the surrounding impls absorb that:

  • Ord for Scalar is partial_cmp(..).unwrap_or(Ordering::Equal), so <, >, cmp, min_by, max_by, and sort_by silently report the values as equal
  • PartialEq for Scalar is partial_cmp(..) == Some(Equal), so == silently reports them as different

Neither raises an error. A size mismatch therefore degrades pruning, read ordering, and overlap checks with no visible signal.

This is reachable today. statistics_to_domain asserted 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-only MODIFY COLUMN that widens a decimal precision without rewriting data.

reduce_column_statistics is the sharpest case. It picked min and max with separate min_by/max_by passes; when comparisons collapse to Equal each pass takes its end from a different input, so the two ends can disagree on their type — which trips the assert! inside ColumnStatistics::new.

Implementation

New meta/stat_cmp.rs:

  • try_cmp_stat_scalars / try_cmp_stat_scalar_slices — compare by scale, then by raw value widened to i256. A decimal's raw value does not depend on its precision, so same-scale values are comparable whatever precision or variant they carry. Return Option<Ordering>.
  • try_stat_ranges_disjoint — range disjointness, None when undecidable
  • retag_stat_scalar / common_stat_decimal_size — reinterpret bounds at a target size for callers that must materialize a value
  • total_cmp_stat_scalars / total_cmp_stat_scalar_slices — a genuine total order for BTreeMap keys, where returning Equal for distinct values would merge map entries

Because comparability depends only on scale, no consumer signature needed a data_type parameter; RuntimeTopNFilter::new and friends are unchanged.

Each consumer picks its own failure direction:

Site On inconclusive
reduce_column_statistics return None, caller omits the column
statistics_to_domain Domain::full
topn_pruner keep every block
RuntimeTopNFilter::boundary_excludes keep the block
cluster_stats_scalar_overlap, scalar_le assume overlap
reduce_cluster_min_max bail out rather than mix ends
sort_by_cluster_stats, rank comparison, to_partitions treat as a tie
check_overlapped_by_stats, table_level_row_prune assume conflict, keep the row
ColumnTopN::find Greater, keeping binary search terminating; the existing recheck rejects a bogus hit
linear_recluster::ScalarSlice total order, so distinct points stay distinct

reduce_column_statistics now aligns all bounds to the widest precision seen before comparing, so both ends agree on one size, and returns Option.

statistics_to_domain retags bounds to the schema size, which let the two debug_assert_eq! on DecimalSize go 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! in range_index.rs while disabling the retag reproduces the real panic:

panicked at range_index.rs:342: assertion `left == right` failed
  left: DecimalSize { precision: 15, scale: 2 }
 right: DecimalSize { precision: 10, scale: 2 }
  • 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 --tests
  • cargo 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 clippy on all five crates, --all-targets -- -D warnings
  • cargo fmt --all -- --check

Two pre-existing failures, verified unrelated by reproducing them with this branch stashed on clean main: storages::system::test_caches_table and test_columns_table (goldenfile drift, only when the full storages:: suite runs). The databend-common-storages-fuse bench target needs /tmp/tpch_1/lineitem.parquet and fails on any checkout.

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

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/max private and does not introduce a view type. It changes one signature (reduce_column_statistics returns Option) 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. The min/max doc comments here name the helpers, but nothing forces their use.

Also worth flagging: the PartialEq collapse direction is the opposite of Ord's, so validate_segment_partition_statistics would report "different partitions" rather than degrade quietly. It is unreachable today because MODIFY COLUMN rejects any column referenced by PARTITION BY, and this PR leaves it untouched rather than adding speculative handling.

AI assistance

  • AI usage: An AI coding agent wrote the comparison helpers, migrated the consumers, and wrote the tests, including verifying each new test fails without the fix. The responsible human owns the review and follow-ups.
  • Responsible human: @youngsofun
  • The responsible human has read every line of this diff and can explain each change

This change is Reviewable

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.
@github-actions github-actions Bot added the pr-bugfix this PR patches a bug in codebase label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

The ## AI assistance section is incomplete. Review is blocked until it is filled in.
@youngsofun please update it 🙏.

  • the checkbox "The responsible human has read every line of this diff and can explain each change" is not checked exactly as written

Required format (see AI_POLICY.md):

## AI assistance

- AI usage: An AI coding agent drafted the patch; I reviewed and added logic tests (or "None")
- Responsible human: @actual-github-id
- [x] The responsible human has read every line of this diff and can explain each change

The responsible human is the author-side owner — the person who has read the diff,
can explain each change, and will answer questions during review.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +158 to +161
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),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge 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 👍 / 👎.

Comment on lines +418 to +420
match self.values.binary_search_by(|entry| {
try_cmp_stat_scalars(&entry.scalar.as_ref(), scalar).unwrap_or(Ordering::Greater)
}) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@youngsofun
youngsofun marked this pull request as draft September 4, 2026 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-bugfix this PR patches a bug in codebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant