expr: don't memoize a fallible operand out of an AND/OR - #37974
Conversation
|
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 |
|
I'm not happy that this is causing known perf. regressions, drafting for now. Edit: Done |
`AND` and `OR` discard an operand's error once another operand fixes the
result: `Or::eval` returns `true` as soon as it sees a `true` operand,
dropping any error it collected, and `And::eval` does the same for
`false`. So an erroring subexpression under one of them is not
observable.
`MapFilterProject::optimize`'s memoization pass descended into those
operands like any other child. A subexpression shared between two of them
therefore became a mapped column, and `SafeMfpPlan::evaluate_inner`
evaluates every mapped column up front, with `?`. That surfaced an error
the expression suppresses, so a row the plan should emit failed the whole
query instead. `MfpPlan::create_from` runs `optimize` unconditionally, so
this is on the path every dataflow operator's linear pipeline takes.
SELECT * FROM t WHERE ((a * a) <= (a * a)) IS NULL OR b IS NULL
with `a` large enough that `a * a` overflows int8 and `b` NULL should
return the row on the strength of the second disjunct. Sharing is what
makes it reachable: a singly-referenced part is inlined back by
`inline_expressions`, so only a part with two or more uses survives as a
column.
Withhold fallible `AND`/`OR` operands from eager memoization, alongside
the `If` branches and `COALESCE` tail already withheld. Infallible ones
stay eligible, which is what keeps the shared comparisons of a null-safe
join predicate in one column each -- gating all operands regressed
`join-implementation`'s plan for `(#0 = #1) OR (#0 IS NULL AND #1 IS
NULL)`. Also withhold `error_if_null`'s message operand, which is only
evaluated when the first operand is NULL and had the same exposure.
Withholding the whole operand costs plan quality wherever a fallible
expression is shared across an AND/OR, so keep the parts whose error every
operand raises. Such a part erroring leaves no operand able to reach the
value that ends the AND/OR, since reaching it means raising the error
first, so the AND/OR errors whether or not the part sits in a column of
its own and hoisting stays sound.
`error_propagating_parts` computes that per operand and
`absorbing_eager_children` intersects across them. Occurring in every
operand is not sufficient and the walk is not syntactic: an operand
holding the part in an `If` branch, a `COALESCE` tail or a nested AND/OR
of its own can reach its value without raising the error, so those
positions do not contribute. The intersection runs only when some operand
is fallible, which leaves the common case on the pre-existing path.
`LirScalarExpr` had neither guard, only the `If` and `COALESCE` cases, and
`MapFilterProject<LirScalarExpr>` reaches `optimize` through `into_plan`:
`LirRelationNode`'s join-closure rewrite in `mz_compute_types::plan`
recomposes `JoinClosure::before` and re-plans it. So the original bug was
live there too, on join predicates, which are exactly where an `OR` over a
fallible comparison shows up. Rather than restate the analysis per
expression type, `OptimizableExpr::error_propagation` classifies a node as
raising its children's errors or absorbing them, and the walk, the
intersection and the eligibility rule are generic over it. Both impls now
carry only their own pattern matches.
This recovers TPC-H and CH-benCHmark q22, `aoc_1219`, `github-5536`, and
in `catalog_server_explain` the `parse_catalog_create_sql` sharing in
`mz_sources` and the `jsonb_array_length` sharing in `mz_clusters` and
`mz_cluster_replicas` -- the plans that run on every environment's catalog
server. What still moves:
* `mz_records_per_dataflow` and `mz_arrangement_sizes_per_worker`, where
a `coalesce(..) + coalesce(..)` sits in one disjunct of a five-way OR
and can genuinely be swallowed by another.
* One case in `literal_constraints.slt`, where `a + b` sits in one
disjunct of `a IN (a + b, a + c, 1 + 5)` and `a = 6` can hand the OR
its `true`. Annotated in place.
* A `record_get` chain in `catalog_server_explain`, which the
intersection is too conservative to keep: it is common to one AND
operand and to a single disjunct of the other, and proving the AND
cannot go `false` needs an analysis of which operands can reach the
absorbing value, not just which raise the error.
Plan time is unchanged within noise. On the slowest query in these files,
`CanonicalizeMfp` totals 194/201/211ms over three runs without the
intersection and 215/214/199/202ms over four with it.
Adds `optimize_preserves_and_or_error_suppression` on both MIR and LIR,
which fails with `Err(NumericFieldOverflow)` without this change,
`optimize_memoizes_a_fallible_part_every_operand_raises` for the
intersection, and `optimize_withholds_a_fallible_part_an_operand_can_skip`
for the `If`-branch case, which fails if the walk descends into branches.
Found by the `mfp_optimize` cargo-fuzz target, which failed
release-qualification 1330, 1332, 1334 and 1335 (v26.35.0-rc.1 through
rc.4) on this.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`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>
|
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. (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 |
Two fixes. The first is a wrong-results / spurious-error bug on the path every
dataflow operator's linear pipeline takes, on both the MIR and the LIR side of
MapFilterProject.AND/ORerror suppression survives MFP memoizationANDandORdiscard an operand's error once another operand fixes theresult:
Or::evalreturnstrueas soon as it sees atrueoperand, droppingany error it collected, and
And::evaldoes the same forfalse. So anerroring subexpression under one of them is not observable.
MapFilterProject::optimize's memoization pass descended into those operandslike any other child. A subexpression shared between two of them therefore
became a mapped column, and
SafeMfpPlan::evaluate_innerevaluates everymapped column up front, with
?. That surfaced an error the expressionsuppresses, so a row the plan should emit failed the whole query instead:
with
alarge enough thata * aoverflows int8 andbNULL should returnthe row on the strength of the second disjunct. Sharing is what makes it
reachable: a singly-referenced part is inlined back by
inline_expressions, soonly a part with two or more uses survives as a column.
The fix withholds fallible
AND/ORoperands from eager memoization,alongside the
Ifbranches andCOALESCEtail already withheld. Infallibleones stay eligible, which is what keeps the shared comparisons of a null-safe
join predicate in one column each.
Keeping the sharing that is still sound
Withholding the whole operand costs plan quality wherever a fallible expression
is shared across an AND/OR, and on the first pass of this PR that moved eleven
goldens, including builtin views that run on every environment's catalog
server. So the fix keeps the parts whose error every operand raises: such a
part erroring leaves no operand able to reach the value that ends the AND/OR,
since reaching it means raising the error first. The AND/OR errors whether or
not the part sits in a column of its own, so hoisting it stays sound.
error_propagating_partscomputes that set per operand andabsorbing_eager_childrenintersects across them. Two things worth flagging forreview:
syntactic. An operand that holds the part in an
Ifbranch, aCOALESCEtail or a nested AND/OR of its own can reach its value without raising the
error, so those positions do not contribute to the set.
error_propagationandeager_childrendescribe the same evaluation andhave to keep agreeing.
eager_childrenanswers "may this part become its owncolumn",
error_propagationanswers "does this part's error reach the top",and the absorbing case is the sole place they differ. Both carry a note.
The same bug on the LIR side
LirScalarExpr::eager_childrenhad neither the AND/OR guard nor theerror_if_nullone, only theIfandCOALESCEcases. That is reachable:MapFilterProject<LirScalarExpr>gets tooptimizethroughinto_plan, whichLirRelationNode's join-closure rewrite inmz_compute_types::plandrives whenit recomposes
JoinClosure::beforeand re-plans it. So the original bug waslive on join closures, which is where an
ORover a fallible comparisonnaturally shows up. I found it by temporarily bounding
optimizeto aMIR-only marker trait and compiling the workspace, which left exactly that one
call site.
Rather than restate a subtle soundness argument once per expression type, the
new
OptimizableExpr::error_propagationclassifies a node as raising itschildren's errors or absorbing them, and the walk, the intersection and the
eligibility rule are generic over it in
mz-expr. Each impl keeps only its ownpattern matches. The LIR test is verified red against dropping the guard.
The intersection runs only when some operand is fallible, so the common case
stays on the pre-existing path. Plan time is unchanged within noise: on the
slowest query in these files,
CanonicalizeMfptotals 194/201/211ms over threeruns without the intersection and 215/214/199/202ms over four with it.
This recovers TPC-H and CH-benCHmark q22,
aoc_1219,github-5536, and incatalog_server_explaintheparse_catalog_create_sqlsharing inmz_sourcesand the
jsonb_array_lengthsharing inmz_clustersandmz_cluster_replicas. Three plans still move:mz_records_per_dataflowandmz_arrangement_sizes_per_worker, where acoalesce(..) + coalesce(..)sits in one disjunct of a five-way OR and cangenuinely be swallowed by another. Correct to withhold.
literal_constraints.slt, wherea + bsits in one disjunct ofa IN (a + b, a + c, 1 + 5)anda = 6can hand the OR itstrue. Correctto withhold, annotated in place.
record_getchain incatalog_server_explain, which is the one place theintersection is too conservative. It is common to one AND operand and to a
single disjunct of the other, so the AND cannot go
falsewhen it errors andhoisting would in fact be sound. Proving that needs an analysis of which
operands can reach the absorbing value rather than which raise the error,
which is a bigger change than this one. Happy to take it if reviewers would
rather not leave it.
Display for EvalErrorpanicked on an unboundedOutOfDomainOutOfDomain(None, None, _)is a state no constructor produces, since all fourcall sites set at least one bound. It is reachable by decoding a corrupted
or forged
ProtoEvalError, becausefrom_protoaccepts any pair of presentProtoDomainLimits andDomainLimit::Noneis a present field with anunbounded value. Renders it instead of panicking.
Tests
optimize_preserves_and_or_error_suppression, on both MIR and LIR, fails withErr(NumericFieldOverflow)without the memoization fix.optimize_memoizes_a_fallible_part_every_operand_raisescovers theintersection.
optimize_withholds_a_fallible_part_an_operand_can_skipcovers theIf-branch case, and fails if the walk descends into branches. Verified redagainst exactly that weakening.
OutOfDomain.The memoization bug was found by the
mfp_optimizecargo-fuzz target, whichfailed release-qualification 1330, 1332, 1334 and 1335 (v26.35.0-rc.1 through
rc.4) on it. That target is sharpened in #37979, which should land after this.