Assert that there are no more projection pushdown opportunities at the end of the MIR pipeline, according to Demand#31808
Assert that there are no more projection pushdown opportunities at the end of the MIR pipeline, according to Demand#31808ggevay wants to merge 1 commit intoMaterializeInc:mainfrom
Demand#31808Conversation
mgree
left a comment
mgree
left a comment
There was a problem hiding this comment.
Fundamentally okay by me, but see my comment. Is the plan to merge just this and drop #31791.
This feature flag boilerplate is a little much---very redundant changes across 6 files. If feature flags are the way forward for now, we'll likely want to find a nicer way to manage the flagging.
| MirRelationExpr::Get { id: Id::Global(id), .. } => { | ||
| let demand = demand_on_ids.get(&Id::Global(*id)).expect("`Demand` should have an opinion on all ids"); | ||
| let actual_proj_above_get = mfp.projection.iter().filter(|c| **c < mfp.input_arity).collect_vec(); | ||
| soft_assert_or_log!( |
There was a problem hiding this comment.
Should relation.pretty() and expr.pretty() and mfp be redacted?
There was a problem hiding this comment.
Good point!
prettyalready redacts in prod:prettycallsExplainConfig::default(), which has
redacted: !mz_ore::assert::soft_assertions_enabled().Display for MapFilterProjectunfortunately doesn't redact, and would be hard to make it do so properly. But sinceDisplay for MapFilterProjectis only used in debugging code, I've now tweaked it to just omit the whole MFP if!soft_assertions_enabled().
|
Thanks for the review!
No, I will merge that one first, then rebase on main, and then merge this one. (I'm currently fighting it out with Nightly over there.) Edit: Merged that one.
Yeah, we could look into this. |
|
Ran Nightly, and the assert is firing in both RQG and full slt! I'll get back to this a bit later. |
Made an issue: https://github.com/MaterializeInc/database-issues/issues/9099. Good luck with the RQG 🫣 |
2 participants
This PR adds a check for an optimizer invariant: There should be no more projection pushdown opportunities at the end of the MIR pipeline, according to
Demand.Compared to what we discussed on Friday with @frankmcsherry and @mgree, the only change is that we skip checking the invariant when there is an
ArrangeByon top of a global Get, because a projection might have been lifted away byJoinImplementationto re-use an index.The first commits are from #31791
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.