Skip to content

expr: don't read unordered stats bounds as an empty range - #38076

Closed
def- wants to merge 1 commit into
MaterializeInc:mainfrom
def-:pr-per-53
Closed

expr: don't read unordered stats bounds as an empty range#38076
def- wants to merge 1 commit into
MaterializeInc:mainfrom
def-:pr-per-53

Conversation

@def-

@def- def- commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

ResultSpec::value_between collapsed min > max to nothing(), i.e. "no value can be here". That only holds for bounds a caller derived coherently. Persist part statistics are not such a caller: arrow orders floats totally, putting -NaN below -Infinity, while OrderedFloat, the Datum order value_between compares in, ranks every NaN above every finite value. A part holding -NaN therefore reports lower = -NaN against a finite upper, the range read back as empty, and filter pushdown discarded the part, losing every other row in it. '-NaN' is ordinary user input, so no corrupt storage is needed to reach this.

Widening unordered bounds in value_between is not sufficient on its own. A part holding NaNs of both signs records the total-order bounds (-NaN, +NaN), which decode under OrderedFloat to NaN == NaN: a seemingly valid, non-inverted range claiming the part holds nothing but NaN. A filter that a finite row matches but NaN does not (for example value = 0.0) still discarded the part. By the time value_between runs, OrderedFloat has erased the NaN signs, and (NaN, NaN) is also the legitimate spec of a genuinely all-NaN part, so the guard has to live in the stats decode, which still sees the sign bits: mz_repr::stats::col_values now refuses to produce float bounds from a negative-NaN lower, unless the upper shows the whole column is NaN.

Tests: unit regressions for both symptoms in persist_source.rs's filter_pushdown_audit module (negative_nan_does_not_discard_matching_part, mixed_sign_nans_do_not_discard_matching_part, each verified to fail without its half of the fix), and peek + dataflow + EXPLAIN FILTER PUSHDOWN cases for both shapes in test/sqllogictest/explain/pushdown.slt.

Closes: PER-53

🤖 Generated with Claude Code

@def-
def- requested review from DAlperin and petrosagg August 6, 2026 11:39
@def-
def- requested review from a team as code owners August 6, 2026 11:39
@def- def- changed the title expr: don't read unordered stats bounds as an empty range (PER-53) expr: don't read unordered stats bounds as an empty range Aug 6, 2026
`ResultSpec::value_between` collapsed `min > max` to `nothing()`, i.e. "no
value can be here". That only holds for bounds a caller derived coherently.
Persist part statistics are not such a caller: arrow orders floats totally,
putting `-NaN` below `-Infinity`, while `OrderedFloat`, the `Datum` order
`value_between` compares in, ranks every NaN above every finite value. A part
holding `-NaN` therefore reports `lower = -NaN` against a finite `upper`, the
range read back as empty, and filter pushdown discarded the part, losing every
other row in it. `'-NaN'` is ordinary user input, so no corrupt storage is
needed to reach this.

Widening unordered bounds is not sufficient on its own. A part holding NaNs
of both signs records the total-order bounds `(-NaN, +NaN)`, which decode to
`NaN == NaN`: a seemingly valid, non-inverted range claiming the part holds
nothing but NaN, so a filter matching a finite row still discarded the part.
Only the stats decode still sees the NaN signs, so `col_values` additionally
refuses to produce float bounds from a negative-NaN lower, unless the upper
shows the whole column is NaN.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q5GiWioZM6CWrwWgfBADDR
@def-

def- commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #38108 (same commit, head branch in this repo so it can take part in a GitHub stack). Bottom of stack #38111: #38108#38109#38110.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant