Skip to content

Commit 7605c6c

Browse files
Remove eager memoization for later arguments to COALESCE (#33109)
Observed in #33103 (comment), and an easy fix. ### 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 b20edd3 commit 7605c6c

File tree

1 file changed

+11
-0
lines changed

1 file changed

+11
-0
lines changed

src/expr/src/linear.rs

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1490,6 +1490,17 @@ pub fn memoize_expr(
14901490
if let MirScalarExpr::If { cond, .. } = e {
14911491
return Some(vec![cond]);
14921492
}
1493+
// We should not eagerly memoize `COALESCE` expressions after the first,
1494+
// as they are only meant to be evaluated if the preceding expressions
1495+
// evaluate to NULL. We could memoize any preceding by expressions that
1496+
// are certain not to error.
1497+
if let MirScalarExpr::CallVariadic {
1498+
func: crate::VariadicFunc::Coalesce,
1499+
exprs,
1500+
} = e
1501+
{
1502+
return Some(exprs.iter_mut().take(1).collect());
1503+
}
14931504
None
14941505
},
14951506
&mut |e| {

0 commit comments

Comments
 (0)