sql: Stable LIR MVP#36544
Conversation
mgree
force-pushed
the
stable-lir-mvp
branch
2 times, most recently
from
0b16743 to
4d7ef6d
Compare
…ar expressions (#36647) ### Motivation https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/20260311_optimizer_customer_tradeoff.md First PR split off of #36544. ### Description As we develop an LIR-level scalar expression, we will need to be able to (a) call `eval()` on those scalars and (b) ask questions about columns. This PR introduces two traits that capture those behaviors, and refactors existing MIR-level MSE functions to work with those abstractions. ### Verification Pure refactor, should be all green.
mgree
force-pushed
the
stable-lir-mvp
branch
9 times, most recently
from
a27cf4c to
719dbe0
Compare
mgree
force-pushed
the
stable-lir-mvp
branch
2 times, most recently
from
cd8dd83 to
4544ccd
Compare
…meter with trait (#36759) ### Motivation Second PR pulled from #36544. ### Description `MapFilterProject` (and its friends, `MfpPlan` and `SafeMfpPlan`---the "MFP" family) are used at both the MIR and LIR level. To separate LIR cleanly from MIR, we need to be able to work with MFPs at both levels. This change parameterizes these types and introduces a trait `OptimizableExpr` that allows us to work independently of the MIR type. It is not the _prettiest_ trait, but I don't see how to work around this (without a much more intensive overhaul of how we process MFP-like structures in LIR). See also: https://linear.app/materializeinc/issue/SQL-319/mfp-like-structures-in-lir-are-optimized-late. ### Verification Pure refactor, CI green.
…meter with trait (#36759) ### Motivation Second PR pulled from #36544. ### Description `MapFilterProject` (and its friends, `MfpPlan` and `SafeMfpPlan`---the "MFP" family) are used at both the MIR and LIR level. To separate LIR cleanly from MIR, we need to be able to work with MFPs at both levels. This change parameterizes these types and introduces a trait `OptimizableExpr` that allows us to work independently of the MIR type. It is not the _prettiest_ trait, but I don't see how to work around this (without a much more intensive overhaul of how we process MFP-like structures in LIR). See also: https://linear.app/materializeinc/issue/SQL-319/mfp-like-structures-in-lir-are-optimized-late. ### Verification Pure refactor, CI green.
antiguru
left a comment
antiguru
left a comment
There was a problem hiding this comment.
Looks good, delta some nits.
| let predicate = MirScalarExpr::CallBinary { | ||
| func: crate::BinaryFunc::Gte(crate::func::Gte), | ||
| expr1: Box::new(mz_now.clone()), | ||
| expr2: Box::new(lb), | ||
| }; |
There was a problem hiding this comment.
Here and just below: Might be easier to read using mz_now.clone().call_binary(crate::func::Gte, lb).
There was a problem hiding this comment.
Done — both bounds now use the builder form: mz_now.clone().call_binary(lb, crate::func::Gte) and …call_binary(ub, crate::func::Lt).
| .filter(|k1| !input_keys.arranged.iter().any(|(k2, _, _)| k1 == &k2)) | ||
| .filter(|k1| { | ||
| !input_keys.arranged.iter().any(|(k2, _, _)| { | ||
| *k1 == &k2.iter().map(MirScalarExpr::from).collect_vec() |
There was a problem hiding this comment.
Could this be k1.iter().eq(k2.iter().map(...))? This might avoid the vector allocation.
There was a problem hiding this comment.
Done, allocation removed. The exact k1.iter().eq(k2.iter().map(MirScalarExpr::from)) does not compile: it needs &MirScalarExpr: PartialEq<MirScalarExpr>, which is not implemented, and Iterator::eq_by is still unstable (iter_order_by). So I kept it allocation-free with a length check plus zip_eq: k1.len() == k2.len() && k1.iter().zip_eq(k2).all(|(e1, e2)| *e1 == MirScalarExpr::from(e2)).
| /// Translates a `&Vec<MirScalarExpr>` (or similar) to a `Vec<LirScalarExpr>`. | ||
| /// | ||
| /// Panics when it encounters unmaterializable functions. | ||
| pub(crate) fn try_lses_from_mses<'a>( |
There was a problem hiding this comment.
I'd expect a try_ function to return a result, not panic. Usually the non-try variant panics, the try variant returns an error.
There was a problem hiding this comment.
Renamed try_lses_from_mses -> lses_from_mses and kept the panic, with a doc comment stating the invariant (LIR-level expressions never contain unmaterializable functions, so the conversion is total; a panic indicates a lowering bug).
I considered making it genuinely fallible (-> Result<Vec<LirScalarExpr>, Vec<UnmaterializableFunc>> with .expect(...) at call sites), but every caller relies on the totality invariant and none could meaningfully propagate the error, so the rename is the better fit. The sibling helpers (mfp_mir_to_lir, safe_mfp_mir_to_lir, mfp_plan_mir_to_lir) already follow this convention — no try_ prefix, panic on unmaterializable funcs — so they are left as-is for consistency.
| let ensure_arrangement = (all_columns, permutation, thinning); | ||
| let ensure_arrangement = (all_columns, permutation.clone(), thinning.clone()); |
There was a problem hiding this comment.
Done — permutation/thinning are moved into the tuple, clones dropped.
| let plan = ThresholdPlan::Basic(BasicThresholdPlan { | ||
| ensure_arrangement: ensure_arrangement.clone(), | ||
| }); | ||
|
|
There was a problem hiding this comment.
Done — removed the stray blank line.
* linear: use call_binary builder for mz_now temporal predicates * lowering: avoid Vec allocation in new_keys filter via iter comparison * threshold: move permutation/thinning into the tuple, drop clones * scalar: rename try_lses_from_mses to lses_from_mses (panics by convention) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Motivation
https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/20260311_optimizer_customer_tradeoff.md
Description
Creates a separate
LirScalarExprstructure; treats MFP-like structures and other auxiliary structures parametrically, introducing several traits (#36759, #36647).Separately spun out #36737 and #36852.
Verification
CI green, nightly green.