[ehigham]

Stack 11. Based on #15934.

Why

ChildEnv.get was one match of 59 instanceof tests. Most nodes (Ref, GetField,
MakeStruct) bind nothing, so they failed every test before reaching case _ => e. The env
walkers call get once per child, and in a profile of TypeCheck on a 1000-field MakeStruct
it took about 7% of samples.

Summary

 get(e, ir, cs, i)(v)
-  ir match                                   // 59 instanceof tests, 2,017 bytes
-    case x: Block => block(e, i, x.bindings)(v)
-    ...
-    case _: TableIR | _: MatrixIR | _: BlockMatrixIR => e.fresh
-    case _ => e
+  (ir.tag.family.toInt: @switch) match       // jump table on families
+    case Family.Stream => getStream(e, ir, cs, i)(v)
+      (ir.tag.toInt: @switch) match          // jump table on the family's nodes
+        case Tag.StreamMap => streamMap(e, i, ir.asInstanceOf[StreamMap].name, cs)(v)
+        ...
+        case _ => e
+    case Family.Table => getTable(e, ir, cs, i)(v)
+      (ir.tag.toInt: @switch) match
+        case Tag.TableFilter => tableFilter(e, i, cs)(v)
+        ...
+        case Tag.TableMultiWrite | Tag.TableCollect | Tag.TableCount | Tag.TableWrite |
+            Tag.TableGetGlobals | Tag.TableToValueApply => e
+        case _ => e.fresh
+    case _ => e                              // Const, Struct, IO, Random bind nothing

Table, Matrix and BlockMatrix mix relational nodes, whose children get e.fresh, with value
nodes, whose children get e. Each of those methods lists its value nodes, the shorter list,
and sends the rest to e.fresh. Every node gets the same env as before.

family	own case	e.fresh	e
Table	9	21	6
Matrix	11	26	4
BlockMatrix	2	10	4

All ten switches compile to a tableswitch. The largest method, getStream, is 538 bytes.

Benchmarks

IRPassBenchmark, 3 forks each, against #15934. Times in µs.

pass	lets	deep	tree	wide
typeCheck	1048 → 980 (−6%)	554 → 530 (−4%)	907 → 837 (−8%)	252 → 233 (−7%)
normalizeNames	1137 → 1095 (−4%)	870 → 845 (−3%)	1408 → 1326 (−6%)	258 → 242 (−6%)
forwardLets	no change	stack overflow in both	2152 → 1805 (−16%)	606 → 569 (−6%)

A second forwardLets run on tree, with the base first, gave −17%. Allocation does not
change. The deep results for typeCheck and normalizeNames are within fork-to-fork noise.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: bb69ba59-89f8-42c3-8261-f76a3596def2

Commits

Reviewing files that changed from the base of the PR and between 55b4335 and 417e48b.

Files selected for processing: 1

• hail/hail/src/is/hail/expr/ir/ChildEnv.scala

No actionable comments were generated in the recent review.

Walkthrough

ChildEnv.get now dispatches IR nodes by family and tag instead of using one type-pattern match. Existing node-specific environment bindings remain. Table, matrix, and block-matrix dispatch also define fallback behavior for unmatched tags.

Changes: ChildEnv dispatch

Layer / File(s)	Summary
Core and stream dispatch hail/hail/src/is/hail/expr/ir/ChildEnv.scala	Core and stream nodes use tag-based dispatch. Existing environment bindings remain, and unmatched core tags return e.
Collection and expression-family dispatch hail/hail/src/is/hail/expr/ir/ChildEnv.scala	Collection, aggregate, NDArray, and relational nodes use family-specific tag dispatch. RunAggScan is handled by the aggregate dispatch.
Table and matrix-family dispatch hail/hail/src/is/hail/expr/ir/ChildEnv.scala	Table, matrix, and block-matrix nodes retain recognized bindings. Specified no-binding operations return e; other unmatched tags in these families return e.fresh.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Possibly related PRs

• hail-is/hail#15919: Introduces ChildEnv, including the per-node environment logic changed here.
• hail-is/hail#15928: Adds ChildEnv.get overloads and walkers that call the dispatch changed here.
• hail-is/hail#15934: Introduces the tag and Family scheme used by this dispatch.

Merge Risk: Minimal · previous review e30e4

No actionable merge-blocking issue is established for the dispatch refactor. Mergeability remains subject to normal build and test checks.

Pre-merge checks: 3 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Description check		The description follows the template's main sections. It gives a Change Description with the motivation, a summary diff, and benchmark results. It has no issue link line, which the template marks as o…	Add the Security Assessment section from the template. State whether the change can impact the Hail Batch instance deployed by Broad Institute in GCP. If it can, choose an Impact Rating and write an Impact Description. For a no-impact or lo…

Passed checks: 3 passed

Check name	Status	Explanation
Title check		The title "[query] Dispatch ChildEnv.get on a per-node tag" matches the main change. The PR replaces the 59-branch instanceof match in ChildEnv.get with dispatch on node family and then on node tag. T…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title "[query] Dispatch ChildEnv.get on a per-node tag" matches the main change. The PR replaces the 59-branch instanceof match in ChildEnv.get with dispatch on node family and then on node tag. The title is specific, short, and names the changed method.

Full details: Description check

Explanation

The description follows the template's main sections. It gives a Change Description with the motivation, a summary diff, and benchmark results. It has no issue link line, which the template marks as optional (delete if N/A). The Security Assessment section is missing entirely. The template requires the author to choose a security-impact option, give an Impact Rating, write an Impact Description, and complete the Appsec Review checkbox when the change may affect the deployed Hail Batch instance. The description does not state whether the change has security impact.

Resolution

Add the Security Assessment section from the template. State whether the change can impact the Hail Batch instance deployed by Broad Institute in GCP. If it can, choose an Impact Rating and write an Impact Description. For a no-impact or low-impact change, a one or two sentence justification is enough. If the change has no impact, the author can delete the Impact Rating, Impact Description, and Appsec Review sections. Optionally, add the issue number to the Change Description line or delete that line.

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: b3abcfb6-6526-41d6-a59f-c8dfa40d6ebc

Commits

Reviewing files that changed from the base of the PR and between 8a8bf3b and e30e4c2.

Files selected for processing: 1

• hail/hail/src/is/hail/expr/ir/ChildEnv.scala

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.