Skip to content

expr: render an unbounded OutOfDomain instead of panicking - #37974

Open
def- wants to merge 1 commit into
mainfrom
def/fuzz-04-expr
Open

expr: render an unbounded OutOfDomain instead of panicking#37974
def- wants to merge 1 commit into
mainfrom
def/fuzz-04-expr

Conversation

@def-

@def- def- commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Display for EvalError panicked on an unbounded OutOfDomain.
OutOfDomain(None, None, _) is a state no constructor produces, since all four
call sites set at least one bound. It is reachable by decoding a corrupted or
forged ProtoEvalError, because from_proto accepts any pair of present
ProtoDomainLimits and DomainLimit::None is a present field with an unbounded
value.

That path is live in clusterd. DataflowErrorSer::Display decodes error bytes
read straight out of a persist shard and delegates to EvalError::fmt, and the
index peek path renders the error-trace key, so one such error in a shard panics
the dataflow and re-panics on every retry. Rejecting the state in from_proto
would not help: DataflowErrorSer::deserialize expects the conversion to
succeed, so the same bytes would panic a few frames earlier on the same path.
Render the degenerate domain instead. Covered by a rendering test, and the
sharpened eval_error_proto_roundtrip oracle in #37979 crashes against the
unfixed Display.

@def- def- changed the title def/fuzz 04 expr expr: don't memoize a fallible operand out of an AND/OR Jul 31, 2026
@def-
def- marked this pull request as ready for review July 31, 2026 09:58
@def-
def- requested a review from a team as a code owner July 31, 2026 09:58
@def- def- mentioned this pull request Jul 31, 2026
Closed
@def-
def- changed the base branch from def/fuzz-03-sql-parser to main July 31, 2026 10:05
@def-
def- requested review from a team as code owners July 31, 2026 10:05
@def-
def- force-pushed the def/fuzz-04-expr branch from 21f8fe1 to 17abc9e Compare July 31, 2026 10:07
@def- def- mentioned this pull request Jul 31, 2026
Draft
@def-
def- requested a review from ggevay July 31, 2026 13:08
@def-

def- commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

We previously had #37049, which had to be reverted! I'm trying to be careful not to repeat this, while still fixing the correctness issues

@def-

def- commented Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I'm not happy that this is causing known perf. regressions, drafting for now. Edit: Done

@def-
def- marked this pull request as draft July 31, 2026 13:24
@def-
def- force-pushed the def/fuzz-04-expr branch from 17abc9e to 579b1d5 Compare July 31, 2026 14:03
@def-
def- marked this pull request as ready for review July 31, 2026 14:08
@def-
def- force-pushed the def/fuzz-04-expr branch 3 times, most recently from 041b411 to 72b1607 Compare August 1, 2026 06:35
@ggevay

ggevay commented Aug 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

I'm not sure if I would consider the AND/OR thing a bug, because we (and Postgres) doesn't really have defined evaluation order for AND/OR. Note that Postgres does promise error guarding for certain constructs (e.g. CASE), but AND/OR is not one of these. See https://linear.app/materializeinc/issue/STG-54/error-semantics-evaluation-order for more details.

(Btw. in the long run, we might be able to have a defined evaluation order for AND/OR that we can actually uphold without performance hits when we implement Datum::Error: At that point, we'll be able to make the memoized expression's error not immediately surface, but just get propagated as Datum::Error, and then the AND/OR could guard whether it's actually surfaced.)

@def-

def- commented Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks Gabor, I'll try to get the fuzzers to ignore this (again). Edit: A small source of panics remained, so left this PR open.

@def- def- closed this Aug 3, 2026
@def- def- reopened this Aug 3, 2026
@def-
def- marked this pull request as draft August 3, 2026 06:29
@def- @claude
expr: render an unbounded OutOfDomain instead of panicking
d0f9240 
`Display for EvalError` panicked on `OutOfDomain(None, None, _)`, a state no
constructor produces: all four `EvalError::OutOfDomain` call sites set at least
one bound. It is reachable by decoding a corrupted or forged `ProtoEvalError`,
because `from_proto` accepts any pair of present `ProtoDomainLimit`s, and
`DomainLimit::None` is a present field with an unbounded value.

That path is live in clusterd. `DataflowErrorSer::Display` decodes error bytes
read straight out of a persist shard and delegates to `EvalError::fmt`, and the
index peek path renders the error-trace key. So one such error in a shard panics
the dataflow, and re-panics on every retry.

Render the degenerate domain instead. Rejecting it in `from_proto` would not
help: `DataflowErrorSer::deserialize` expects the conversion to succeed, so the
same bytes would panic a few frames earlier on the same path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@def-
def- force-pushed the def/fuzz-04-expr branch from 72b1607 to d0f9240 Compare August 3, 2026 07:27
def- added a commit that referenced this pull request Aug 3, 2026
@def- @claude
tests: skip absorbed AND/OR operand errors row-precisely in mfp_optimize
34621ed 
`MapFilterProject::optimize` may legally change what a row yields once
lazy `And`/`Or` evaluation absorbs an operand error, in two ways. Reduce's
error propagation folds the absorbed error into the plan (the open bug
CLU-137), and memoization hoists a fallible subexpression shared under the
`And`/`Or` into a mapped column that `SafeMfpPlan::evaluate_inner` runs
eagerly, surfacing the error. The latter is not a bug: AND/OR evaluation
order is undefined, in Materialize as in Postgres (STG-54), so the
surfaced error is a legal outcome of the same plan. It is what failed
release qualification 1330, 1332, 1334 and 1335, and after review #37974
no longer changes the product to prevent it, so the oracle has to
tolerate it or release qualification keeps tripping on it.

The coarse expression-level skip this target used drops the entire input
whenever any `And`/`Or` has a statically fallible operand. That is 2.2% of
MFPs, but they are exactly the AND/OR-over-fallible shapes where
optimize's memoization is busiest, so the value and filter arms went
blind where a real miscompile is most likely to sit. Port
`mir_scalar_reduce`'s row-precise `absorbs_and_or_operand_error` instead
and skip exactly the rows whose lazy evaluation absorbs an operand error.
Temporal mode keeps the coarse skip: its reference side substitutes a
literal for `mz_now()` and re-runs `optimize` per probed time, so a
per-row answer is not well-defined there.

Verified in both directions against the unfixed product. A hand-crafted
input generating `((#2 * #2) <= (#2 * #2)) IS NULL OR #4` with an
overflowing `#2` crashes the pre-existing oracle with the release
qualification assertion and passes the row-precise one. A twin input
hiding the shared fallible part in an untaken `If` branch still crashes
the row-precise oracle when `eager_children`'s `If` guard is deliberately
removed, so the tolerance does not extend to genuine eager-evaluation
bugs.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@def- def- changed the title expr: don't memoize a fallible operand out of an AND/OR expr: render an unbounded OutOfDomain instead of panicking Aug 3, 2026
@def-
def- marked this pull request as ready for review August 3, 2026 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggevay ggevay Awaiting requested review from ggevay

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@def- @ggevay