Skip to content

expr: fix persist filter pushdown interpreter soundness bugs (PER-50)#37638

Merged
DAlperin merged 1 commit into
MaterializeInc:mainfrom
DAlperin:dovalperin/per-50-thread-timelywork-2-panicked-at-srcstorage
Jul 15, 2026
Merged

expr: fix persist filter pushdown interpreter soundness bugs (PER-50)#37638
DAlperin merged 1 commit into
MaterializeInc:mainfrom
DAlperin:dovalperin/per-50-thread-timelywork-2-panicked-at-srcstorage

Conversation

@DAlperin

@DAlperin DAlperin commented Jul 14, 2026

Copy link
Copy Markdown
Member

Motivation

Fixes PER-50 (database-issues#9656): persist filter pushdown correctness violation panics.

The ColumnSpecs abstract interpreter decides whether persist filter pushdown can skip fetching a part, based on the part's column statistics. For a function marked is_monotone it samples only the endpoints of an input range and unions the results. That is sound for the function's output value, but the code trusted those two samples for more than monotonicity guarantees, and a few domain operations had gaps. Each produced a false negative: a part wrongly proven unable to match, so pushdown discards it and the runtime audit panics. Without the audit it would silently drop matching or erroring rows.

I found the first bug with a new range-based soundness proptest, then that plus an end-to-end stats→filter_result proptest surfaced the rest.

Bugs fixed (src/expr/src/interpret.rs)

  • Interior fallibility. A could_error function can error on an interior input the endpoints miss (round(numeric, scale) overflows on an exponent-dependent condition, numeric::mz_timestamp rejects fractional inputs), and fallibility was read off the endpoint evaluations. A could_error function is now fallible over any multi-valued input range.
  • NaN as an input bound. NaN sorts as the max of the numeric/float order but is a fixed point of monotone functions, breaking endpoint narrowing. The shortcut is skipped when a bound is NaN.
  • NaN as an output. Mul/Div with an infinite operand are not corner-sampleable (inf * 0, inf / inf = NaN, and the reachable value lives in the interior, e.g. [-inf, +inf] * 0 maps both endpoints to NaN but the finite interior to 0). They fall back to the full value domain when an operand may be infinite.
  • If with a null condition. eval takes the els branch on False and Null, but the interpreter mapped a null condition to fails(), dropping els. Null now takes els.
  • Values::intersect of a range and a map spec. Datum order places Map between List and Numeric, so a Within range straddling those tags contains map values; intersecting it with a Nested spec returned Empty, dropping common values. It now returns the Nested side.
  • The monotone-narrowing arm now treats value / null / error as orthogonal channels, so surfacing an operand's fallibility no longer collapses the value range of an enclosing monotone op.

Tests

  • test_equivalence_ranges: the equivalence proptest was only ever fed single-value or unconstrained specs, so it never exercised the range narrowing production stats drive. This variant feeds genuine value_between ranges.
  • gen_expr_for_relation now generates If nodes; type/function coverage widened (float, interval, int32, float arithmetic, round, temporal casts).
  • test_result_spec_lattice_laws: union/intersect soundness of the abstract domain.
  • filter_pushdown_audit (persist_source.rs): end-to-end proptest that builds a part from real rows, computes the real production stats, and asserts filter_result never discards a part whose MFP produces output on an actual row. This is the only layer that also catches stats-derivation bugs.
  • Deterministic regressions for the numeric-NaN and interior-error cases.

Behavior change

Pushdown is now more conservative for predicates that run a column through an erroring function (arithmetic, casts, interval math): such a part is kept rather than pruned. Plain comparison filters are unaffected. The notable loss is temporal-window pruning built from interval or division arithmetic. Recovering that precisely is a follow-up: make the fallibility annotation argument-aware so, for example, col / 10 with a constant nonzero divisor stays prunable.

@DAlperin DAlperin marked this pull request as ready for review July 14, 2026 15:08
@DAlperin DAlperin requested review from a team as code owners July 14, 2026 15:08
@DAlperin DAlperin requested review from antiguru and def- July 14, 2026 15:08

@antiguru antiguru left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great!

Comment thread src/expr/src/interpret.rs
Comment on lines +1159 to +1161
if fallible && inputs_multivalued {
range.fallible = true;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be good to leave a comment here. The line above seems to indicate that fallibility is already included in the intersect call, but that doesn't seem to be the case, and at least to me it's not clear why we need the if block.

Comment thread src/expr/src/interpret.rs Outdated
// excluded: `NaN` is ordered as the maximum but is a fixed point of
// most monotone functions, so evaluating the endpoints does not
// bound the interior. Such ranges fall through to the
// overapproximation below. See database-issues#9656.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure how much value we add by referencing the issue. Consider not mentioning it.

@DAlperin DAlperin force-pushed the dovalperin/per-50-thread-timelywork-2-panicked-at-srcstorage branch from 071728d to ab4960d Compare July 14, 2026 15:26
The `ColumnSpecs` abstract interpreter decides whether persist filter
pushdown can skip fetching a part, based on the part's column statistics.
For a function marked `is_monotone` the interpreter samples only the
endpoints of an input range and unions the results. That is sound for the
function's output *value*, but the code trusted those two samples for more
than monotonicity guarantees, and a few domain operations had gaps. Each
produced a false negative: a part wrongly proven unable to match, so
pushdown discards it and the runtime audit panics ("persist filter
pushdown correctness violation"). Without the audit it would silently drop
matching or erroring rows.

Bugs fixed in `ColumnSpecs` / `ResultSpec` (src/expr/src/interpret.rs):

- Interior fallibility. A `could_error` function can error on an input in
  the interior of a range that the endpoints don't hit, and the code read
  fallibility off the endpoint evaluations. `round(numeric, scale)`
  overflows depending on the input's decimal exponent (not its magnitude),
  and `numeric::mz_timestamp` rejects fractional inputs, so both error
  between two non-erroring bounds. A `could_error` function is now treated
  as fallible over any multi-valued input range.

- NaN as an input bound. `NaN` sorts as the maximum of the numeric/float
  order but is a fixed point of most monotone functions, so a range whose
  bound is `NaN` breaks endpoint narrowing. The monotone shortcut is now
  skipped when a bound is `NaN`.

- NaN as an output. Multiplication and division with an infinite operand
  are not corner-sampleable: `inf * 0` and `inf / inf` evaluate to `NaN`,
  and the value they take can be reached only from the interior (e.g.
  `[-inf, +inf] * 0` maps both endpoints to `NaN` while the finite interior
  maps to `0`). Those operations now fall back to the full value domain
  when an operand may be infinite. This is driven by a new
  `is_infinity_monotone` property on the `sqlfunc` macro (set `false` for
  multiplication and division) rather than a hardcoded function list.

- `If` with a null condition. `MirScalarExpr::eval` takes the `els` branch
  on both `False` and `Null`, but the interpreter mapped a null condition
  to `fails()`, dropping `els` from the value channel. Null now takes `els`.

- `Values::intersect` of a range and a map spec. `Datum` order places `Map`
  between `List` and `Numeric`, so a `Within` range straddling those tags
  contains map values. Intersecting such a range with a `Nested` (map) spec
  returned `Empty`, dropping values in both. It now returns the `Nested`
  side as a sound over-approximation.

- The monotone-narrowing arm now treats the value, null, and error channels
  orthogonally, so surfacing an operand's fallibility no longer collapses
  the value range of an enclosing monotone operation to "anything".

Tests (src/expr/src/interpret.rs, src/storage-operators/src/persist_source.rs):

- `test_equivalence_ranges`: the equivalence proptest was only ever fed
  single-value or unconstrained column specs, so it never exercised the
  range narrowing that production stats drive. This variant feeds genuine
  `value_between` ranges with an interior row value.
- `gen_expr_for_relation` now generates `If` nodes.
- `test_result_spec_lattice_laws`: union/intersect soundness of the
  abstract domain in isolation.
- The generator's type and function coverage is widened (float, interval,
  int32, float arithmetic, round, temporal casts) so the fixes above are
  exercised beyond `Numeric`.
- `filter_pushdown_audit`: an end-to-end proptest that builds a part from
  real rows, computes the real production statistics, and asserts
  `filter_result` never discards a part whose MFP produces output on an
  actual row. This is the only layer that catches stats-derivation bugs as
  well as interpreter bugs.
- Deterministic regressions for the numeric-NaN and interior-error cases.

Behavior change: pushdown is now more conservative for predicates that run
a column through an erroring function (arithmetic, casts, interval math):
such a part is kept rather than pruned. Plain comparison filters are
unaffected. The notable loss is temporal-window pruning built from interval
or division arithmetic. Recovering that precisely is a follow-up: make the
fallibility annotation argument-aware so, for example, `col / 10` with a
constant nonzero divisor stays prunable.
@DAlperin DAlperin force-pushed the dovalperin/per-50-thread-timelywork-2-panicked-at-srcstorage branch from ab4960d to 8896e24 Compare July 14, 2026 15:32
@DAlperin DAlperin merged commit d08f8f7 into MaterializeInc:main Jul 15, 2026
121 checks passed
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.

2 participants