Skip to content

Commit dd50ff0

Browse files
Remove error promotion from COALESCE (#33103)
We have an expression optimization that promotes literal errors around the documented evaluation order guards of `COALESCE`. It seems like this may be for "postgres compatibility", as postgres does a similar thing of erroring even when there are non-null values before the error (but only observed when the error is present in the query text, rather than as a result of data). We have situations where literal errors result from data, and need to guard the promotion of the error in the way that `COALESCE` is documented to do. Seems hard to imagine fall-out from this, as we believe we are optimizing the performance of an error-producing case. We'll see what tests say! ### Motivation <!-- Which of the following best describes the motivation behind this PR? * This PR fixes a recognized bug. [Ensure issue is linked somewhere.] * This PR adds a known-desirable feature. [Ensure issue is linked somewhere.] * This PR fixes a previously unreported bug. [Describe the bug in detail, as if you were filing a bug report.] * This PR adds a feature that has not yet been specified. [Write a brief specification for the feature, including justification for its inclusion in Materialize, as if you were writing the original feature specification.] * This PR refactors existing code. [Describe what was wrong with the existing code, if it is not obvious.] --> ### Tips for reviewer <!-- Leave some tips for your reviewer, like: * The diff is much smaller if viewed with whitespace hidden. * [Some function/module/file] deserves extra attention. * [Some function/module/file] is pure code movement and only needs a skim. Delete this section if no tips. --> ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. ([trigger-ci for additional test/nightly runs](https://trigger-ci.dev.materialize.com/)) - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. <!-- Reference the design in the description. --> - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)). <!-- Ask in #team-cloud on Slack if you need help preparing the cloud PR. --> - [ ] If this PR includes major [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note), I have pinged the relevant PM to schedule a changelog post.
1 parent cee85b6 commit dd50ff0

File tree

1 file changed

+1
-19
lines changed

1 file changed

+1
-19
lines changed

src/expr/src/scalar.rs

Lines changed: 1 addition & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1237,11 +1237,7 @@ impl MirScalarExpr {
12371237
let mut prior_exprs = BTreeSet::new();
12381238
exprs.retain(|e| prior_exprs.insert(e.clone()));
12391239

1240-
if let Some(expr) = exprs.iter_mut().find(|e| e.is_literal_err()) {
1241-
// One of the remaining arguments is an error, so
1242-
// just replace the entire coalesce with that error.
1243-
*e = expr.take();
1244-
} else if exprs.len() == 1 {
1240+
if exprs.len() == 1 {
12451241
// Only one argument, so the coalesce is a no-op.
12461242
*e = exprs[0].take();
12471243
}
@@ -3400,13 +3396,6 @@ mod tests {
34003396
},
34013397
output: lit(1),
34023398
},
3403-
TestCase {
3404-
input: MirScalarExpr::CallVariadic {
3405-
func: VariadicFunc::Coalesce,
3406-
exprs: vec![col(0), err(EvalError::DivisionByZero)],
3407-
},
3408-
output: err(EvalError::DivisionByZero),
3409-
},
34103399
TestCase {
34113400
input: MirScalarExpr::CallVariadic {
34123401
func: VariadicFunc::Coalesce,
@@ -3418,13 +3407,6 @@ mod tests {
34183407
},
34193408
output: err(EvalError::DivisionByZero),
34203409
},
3421-
TestCase {
3422-
input: MirScalarExpr::CallVariadic {
3423-
func: VariadicFunc::Coalesce,
3424-
exprs: vec![col(0), err(EvalError::DivisionByZero)],
3425-
},
3426-
output: err(EvalError::DivisionByZero),
3427-
},
34283410
];
34293411

34303412
for tc in test_cases {

0 commit comments

Comments
 (0)