[query] no-sharing in MatrixIR lowering#15513
Conversation
d43d087 to
b98c707
Compare
2817262 to
c7ad1e1
Compare
MatrixIR lowering
c7ad1e1 to
16a34ad
Compare
69d3574 to
a400d2c
Compare
1bc0440 to
7dd6844
Compare
| maxLen: Int = -1, | ||
| allowUnboundRefs: Boolean = false, | ||
| preserveNames: Boolean = false, | ||
| preserveNames: Boolean = true, |
There was a problem hiding this comment.
Calling this out - preserveNames = true is a much more useful default for me.
For lowering invariants, you want to see where a name is bound thoughout a stacktrace. After extract, you want to identify the subexpression that was lifted, etc.
b84c6ad to
efff260
Compare
457da83 to
73f77ad
Compare
73f77ad to
deb6043
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates MatrixIR lowering and related IR-building utilities to preserve the TreeIR “no-sharing / name-normalized” invariant throughout matrix-to-table lowering, as part of the broader effort to reduce repeated NormalizeNames invocations and eventually remove noSharing.
Changes:
- Refactors
LowerMatrixIRto avoidsubst-based rewriting and instead introduce bindings via scoped shadowing (with a finalNormalizeNameson the result). - Updates
DeprecatedIRBuilderto use a fullBindingEnv(eval/agg/scan) so that shadowing works correctly across binding scopes. - Simplifies/rewrites multiple
MatrixWriterlowerings (e.g., usingMemoized, removing intermediates), plus small cleanups across tests and pretty-printing defaults.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| hail/hail/test/src/is/hail/HailSuite.scala | Deep-copies BlockMatrixIR before running multiple exec strategies to avoid accidental sharing/mutation. |
| hail/hail/test/src/is/hail/expr/ir/TableIRSuite.scala | Updates MatrixNativeReader usage to the new spec accessor. |
| hail/hail/test/src/is/hail/expr/ir/table/TableGenSuite.scala | Simplifies lowering tests to evaluate collected IR directly; adopts newer IR helpers (makestruct, MakeStream.single). |
| hail/hail/test/src/is/hail/expr/ir/MatrixIRSuite.scala | Uses Atom.ir deep copies in test IR construction to avoid sharing. |
| hail/hail/test/src/is/hail/expr/ir/Aggregators2Suite.scala | Refactors aggregator IR construction using higher-level helpers (insertIR, aggBindIR). |
| hail/hail/src/is/hail/expr/ir/Pretty.scala | Changes Pretty default to preserveNames = true; adjusts SSA arg naming for MatrixMapRows. |
| hail/hail/src/is/hail/expr/ir/Optimize.scala | Relies on new Pretty defaults (drops explicit preserveNames = true). |
| hail/hail/src/is/hail/expr/ir/MatrixWriter.scala | Updates writer components to accept Atom bindings; rewrites several writer lowerings with Memoized and helper combinators. |
| hail/hail/src/is/hail/expr/ir/MatrixIR.scala | Introduces MakeArray.empty; removes getSpec() in favor of spec; refactors native reader col-reading IR. |
| hail/hail/src/is/hail/expr/ir/LowerMatrixIR.scala | Major refactor to shadow names per-scope and enforce name-normalized output via NormalizeNames. |
| hail/hail/src/is/hail/expr/ir/lowering/LowerTableIR.scala | Updates TableStage APIs to accept Atom and reduces repeated evaluation with bindIR. |
| hail/hail/src/is/hail/expr/ir/lowering/LoweringPass.scala | Simplifies LowerMatrixToTablePass to call the unified LowerMatrixIR entrypoint; tightens before invariant. |
| hail/hail/src/is/hail/expr/ir/lowering/LowerBlockMatrixIR.scala | Minor cleanup using MakeStream.single. |
| hail/hail/src/is/hail/expr/ir/lowering/invariant/package.scala | Uses new Pretty defaults for invariant failure traces. |
| hail/hail/src/is/hail/expr/ir/IR.scala | Makes Let.apply elide empty blocks; adds MakeArray.empty(elementType). |
| hail/hail/src/is/hail/expr/ir/ForwardLets.scala | Uses new Pretty defaults for logging. |
| hail/hail/src/is/hail/expr/ir/DeprecatedIRBuilder.scala | Switches to BindingEnv[Type] and updates agg/scan/eval environment handling for shadowing correctness. |
| hail/hail/src/is/hail/expr/ir/agg/Extract.scala | Fixes init-op arg binding behavior when knownLength is present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case MatrixColsHead(child, n) => | ||
| lower(ctx, child, liftedRelationalLets) | ||
| .mapGlobals('global.insertFields('__cols -> 'global('__cols).arraySlice(0, Some(n), 1))) | ||
| .mapRows('row.insertFields(entriesField -> 'row(entriesField).arraySlice(0, Some(n), 1))) |
| case MatrixColsTail(child, n) => | ||
| lower(ctx, child, liftedRelationalLets) | ||
| .mapGlobals('global.insertFields('__cols -> 'global('__cols).arraySlice(-n, None, 1))) | ||
| .mapRows('row.insertFields(entriesField -> 'row(entriesField).arraySlice(-n, None, 1))) |
Partially resolves #13250
Succeeds #15520
This change is one in a series that aims to preserve the TreeIR invariant throught the various lowerings, culminating in the removal of noSharing.
In this change,
MatrixIRlowerings generate a name-normalised tree-ir.This is enforced via the
beforeandafterinvariants inLowerMatrixToTablePass.The major changes include:
LowerMatrixIR:substwith a particularBindingEnvDeprecatedIRBuilder:BindingEnvas shadowing names in diffenent binding scopes requires separate environmentsMatrixWriterMemoizedto reduce indent.MatrixBlockMatrixWriter)MatrixBGENWriter)__freshNameas it helps with Pretty output (especially withpreserveNames = true)makestuct(..)overMakeStruct(FastSeq(..))