Remove error promotion from COALESCE

[frankmcsherry]

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

Tips for reviewer

Checklist

• This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
• This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
• If this PR evolves an existing $T ⇔ Proto$T mapping (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).
• If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

[frankmcsherry]

A point worth discussing: this breaks PostgreSQL compatibility for a query like SELECT COALESCE(a, 1/0), where PostgreSQL will return an error even though "not evaluating" the 1/0. At the same time, this fixes a PostgreSQL compatibility error where it could return a correct answer and Materialize will error:

Postgres

mcsherry=# create table test (a int, b int);
CREATE TABLE
mcsherry=# insert into test values (1, 0);
INSERT 0 1
mcsherry=# select coalesce(a, 1/b) from test where b = 0;
 coalesce 
----------
        1
(1 row)

mcsherry=# 

Materialize

materialize=> create table test (a int, b int);
CREATE TABLE
materialize=> insert into test values (1, 0);
INSERT 0 1
materialize=> select coalesce(a, 1/b) from test where b = 0;
ERROR:  Evaluation error: division by zero
materialize=> 

The only way I can square this is that PostgreSQL replaces queries with errors if literal errors can occur in the query text, but then turns off that part of its brain in the course of query optimization. If we really want to respect this form of compatibility, it feels like a pass before any query transformations have occured.

[ggevay]

The Postgres docs say coalesce should short-circuit. So, I wouldn't try to match Postgres in those cases where it doesn't short-circuit, because these seem to be bugs in Postgres itself. So, I'd say the PR just fixes a bug where Materialize was also not short-circuiting, which is a good thing.

I did some more tests:

create table test (a int, b int);
insert into test values (1, 0);

# 1. main: ok,  PR: ok, Postgres: ok
select coalesce(a, 1/b) from test;

# 2. main: err, PR: ok, Postgres: err
select coalesce(a, 1/0) from test;

# 3. main: err, PR: ok, Postgres: ok
select coalesce(a, 1/b) from test where b = 0;

# 4. main: ok,  PR: ok, Postgres: ok
select coalesce(7, 1/b) from test;

# 5. main: ok,  PR: ok, Postgres: ok
select coalesce(7, 1/0) from test;

create table test_nonnull (a int not null, b int);
insert into test_nonnull values (1, 0);

# 1-nonnull. main: ok,  PR: ok, Postgres: ok
select coalesce(a, 1/b) from test_nonnull;

# 2-nonnull. main: ok,  PR: ok, Postgres: err
select coalesce(a, 1/0) from test_nonnull;

# 3-nonnull. main: ok,  PR: ok, Postgres: ok
select coalesce(a, 1/b) from test_nonnull where b = 0;

# 4-nonnull. main: ok,  PR: ok, Postgres: ok
select coalesce(7, 1/b) from test_nonnull;

# 5-nonnull. main: ok,  PR: ok, Postgres: ok
select coalesce(7, 1/0) from test_nonnull;


# Still buggy. main: err, PR: err, Postgres: ok
select coalesce(a, a/b + 1, a/b + 2) from test;

I'll add these in an slt and push to the PR.

Unfortunately, I can still make coalesce not short-circuit, see the last example above, where we have the plan:

                        Physical Plan                         
--------------------------------------------------------------
 Explained Query:                                            +
   →Read materialize.public.test                             +
                                                             +
 Source materialize.public.test                              +
   project=(#3)                                              +
   map=((#0{a} / #1{b}), coalesce(#0{a}, (#2 + 1), (#2 + 2)))+

We'd need to add a similar exception in MFP CSE for coalesce as we have for MSE::If:

materialize/src/expr/src/linear.rs

Lines 1488 to 1492 in 98cf9ae

	// We should not eagerly memoize `if` branches that might not be taken.
	// TODO: Memoize expressions in the intersection of `then` and `els`.
	if let MirScalarExpr::If { cond, .. } = e {
	return Some(vec![cond]);
	}

I've added a note in the error semantics issue: https://github.com/MaterializeInc/database-issues/issues/4972 But also, this looks like an easy fix for a quick follow-up PR.

[frankmcsherry]

We'd need to add a similar exception in MFP CSE for coalesce as we have for MSE::If:

This sounds worth doing and I'll take a look. I don't think we need extensive testing done yet though, in the absence of clear semantics (we had two tests for the previous version; the bug was that we didn't know what was supposed to happen). No harm, but it's more stuff we'll have to review once we do have an opinion on the error semantics (which @mgree is peeking at, so it's not wildly far-fetched).

[frankmcsherry]

Maybe said differently: I think the tests for "postgres compat" make sense, but we should keep them apart from "intended behavior". Having just deleted some of the "intended behavior" tests for not actually being the intended behavior, but rather inspired by postgres compat. :D

[ggevay]

I think it's good to add tests, so that we at least know about it when the behavior changes. We'll need these tests anyway later when we do more comprehensive work on error semantics. Also, this is input to @mgree's work: These are some specific example queries that he can look at to see what his formal semantics say on them.

I've pushed the tests here: Could you please cherry-pick that commit? ggevay@0d2b6ae

Edit: Could you also add a comment above the new tests that we haven't totally decided yet whether this is the intended behavior?

[frankmcsherry]

I think it's good to add tests, so that we at least know about it when the behavior changes.

So, we don't agree on this. Let's discuss async.

[ggevay]

LGTM!