Avoid deconstructing temporal filters in memoize_expr

[ggevay]

Fixes https://github.com/MaterializeInc/database-issues/issues/9643 by making memoize_expr (which is called from extract_common) not go into temporal filters.

Note that inline_expressions solves this problem when full MFP optimization is run, but extract_common only runs memoize_expressions.

The PR adds a regression test in temporal.slt. (temporal.td also tests various temporal filters.) Additionally, the requirement that the non-mz_now side of a temporal filter should still be processed by memoize_expressions is tested by the existing EXPLAINs in filter-pushdown.slt.

The PR also makes the existing tests slightly stronger by putting actual data into one of the existing tables, and actually querying the existing materialized views to check for evaluation errors, in addition to planning errors.

Nightly (subset): https://buildkite.com/materialize/nightly/builds/13060

Edit: I've thought about feature flagging this change, but unfortunately the optimizer flags are not wired to this part of the code. But I think this is a low-risk change.

New Nightly run: https://buildkite.com/materialize/nightly/builds/13227

Motivation

• This PR fixes a recognized bug: https://github.com/MaterializeInc/database-issues/issues/9643

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.

[ggevay]

I'm also open to refactoring these into one method if deemed worthwhile.

[mgree]

Eh, seems fine as is. Maybe worth adding a comment that temporal filters are necessarily binary operations?

[ggevay]

Eh, seems fine as is.

Ok, I'm relieved :)

Maybe worth adding a comment that temporal filters are necessarily binary operations?

Below the let is_temporal_filter = line, there is the following old comment:

// Supported temporal predicates are exclusively binary operators.

Or did you mean in the doc comment of the fn is_temporal_filter? That kinda has it already, albeit somewhat implicitly, right?

[ggevay]

(I ended up factoring this out, due to Frank's comment.)

[mgree]

Minor nits---looks good to me!

[mgree]

Eh, seems fine as is. Maybe worth adding a comment that temporal filters are necessarily binary operations?

[frankmcsherry]

To discuss: why not use MapFilterProject::extract_temporal, and ultimately SafeMfpPlan to go from MFPs containing temporal operators to those with and without. (( i.e. I can't tell why the approach here is the right solution, vs using existing methods ))

[frankmcsherry]

Unless there is a rush, I think there's potentially a clearer way to do this, where we centralize the "am I a temporal filter" logic, deduplicating that and removing the need for tests that the two bits of logic agree.

[frankmcsherry]

Nit, but naming-wise I would expect is_foo(&mut self) -> ... to return a bool and not be &mut. Perhaps instead as_mut_temporal_filter_expr? That's a bit horrid, but probably clearer when used and less surprising.

[frankmcsherry]

Reading the other changes, I think what we actually want here is

/// If `self` expresses a temporal filter, normalize it to start with `mz_now()` and return references.
/// 
/// A temporal filter is an expression of the form `mz_now() <BINOP> <EXPR>`,
/// for a restricted set of `BINOP` and `EXPR` that do not themselves contain `mz_now()`.
/// Expressions may conform to this once their expressions are swapped.
///
/// If the expression is not a temporal filter it will be unchanged.
pub fn as_mut_temporal_filter(&mut self) -> Option<(&BinaryFunc, &mut MirScalarExpr)>

which I think centralizes the logic in one place, rather than have two flavors and tests to make sure they stay the same.

[ggevay]

Yes, ok, this is what I meant in https://github.com/MaterializeInc/materialize/pull/33500/files#r2318981605
If you deem this refactoring worthwhile, then I'll do it.

[ggevay]

Done, see the latest commit.

[frankmcsherry]

The code changes look great, thank you!

[ggevay]

Thanks for the review! (The Nightly fail looks unrelated.)