Extract Lets from LetRec
#30974
Extract Lets from LetRec
#30974
Conversation
| Join on=(#2 = (#0 % 2)) type=differential // { arity: 3 } | ||
| implementation | ||
| %1:l0[#0]UK » %0:t2[(#0 % 2)]K | ||
| ArrangeBy keys=[[(#0 % 2)]] // { arity: 2 } | ||
| ReadStorage materialize.public.t2 // { arity: 2 } | ||
| ArrangeBy keys=[[#0]] // { arity: 1 } | ||
| With Mutually Recursive | ||
| cte l1 = | ||
| Distinct project=[#0..=#2] // { arity: 3 } | ||
| Union // { arity: 3 } | ||
| Get l1 // { arity: 3 } | ||
| Project (#0, #0, #0) // { arity: 3 } | ||
| Get l0 // { arity: 1 } |
There was a problem hiding this comment.
Who knows, but potentially redundant join elimination not operating in LetRec blocks?
frankmcsherry
force-pushed
the
extract_lets_from_letrec
branch
from
5304bc2 to
b77fddc
Compare
| // This is unsafe when applied to expressions which contain `ArrangeBy`, | ||
| // as if the extracted suffixes reference arrangements they will not be | ||
| // able to access those arrangements from outside the `LetRec` scope. | ||
| // It happens to work at the moment, so we don't touch it but should fix. | ||
| let bindings = let_motion::harvest_nonrec_suffix(relation)?; |
There was a problem hiding this comment.
When the new code above wraps a LetRec into a Let, this can make the harvest_nonrec_suffix call ineffective, because relation is no longer a LetRec, but harvest_nonrec_suffix just wants to match LetRec at the root. This seems to be happening in the output change in normalize_lets.spec below, where l2 used to come out of the WMR, but now it remains in there (and similarly in aoc_1204.slt with l7, and in aoc_1203.slt with all of l10 .. l19 of the old version). Would it be possible to tweak the code to be able to still do harvest_nonrec_suffix in this case? It might be as simple as doing the harvest_nonrec_suffix call and the below code block not on relation, but on the innermost relation of the Let stack that is being assembled above?
Our let normalization consolidates let and letrec stages, partly on account of tracking the same thing two different ways is annoying and makes the code complicated. At the same time, other parts of the code prefer to have as much extracted from letrec stages as possible, both because transforms can be less effective with letrec stages, and because once we have
ArrangeBystages they should be hoisted as high out of letrec stages as is possible, to maximize their availability.Both of these feel like glitches that we should solve closer to the problems (e.g. improving the transforms to achieve parity, and hoisting arrangements in lowering to renderable plans). This PR is first to see what the plan changes are and whether we like them, and if we are lucky to address some complaints we have around arrangements not being found come render time.
cc: @ggevay
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.