[ehigham]

createAgg copied eval into agg but also emptied scan; createScan emptied agg. So creating both scopes, binding in each as you go, loses the first scope's bindings:

  env.createAgg.bindAgg(x -> t).createScan
    before: agg = Some({}),      scan = Some(eval)
    after:  agg = Some({x: t}),  scan = Some(eval)

extend's AggEnv.Create already copies eval and leaves the other scope alone. createAgg and createScan now do the same.

This change does not affect batch.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ 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: 545f7dad-775f-4d3f-a5c0-e10a41d199c9

📥 Commits

Reviewing files that changed from the base of the PR and between 06cacaf and ec3fb78.

📒 Files selected for processing: 1

• hail/hail/src/is/hail/expr/ir/Env.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.

📝 Walkthrough

BindingEnv.createAgg and createScan now set their respective environments from eval without clearing the opposite-scope environment.

Changes: BindingEnv scope creation

Layer / File(s)	Summary
Scope environment initialization hail/hail/src/is/hail/expr/ir/Env.scala	createAgg and createScan set their own environment from eval and leave the opposite-scope environment unchanged.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~5 minutes

Change: Bug fix

Possibly related PRs

• hail-is/hail#15855: Both PRs change BindingEnv scope handling for aggregation and scan lifting.

Suggested reviewers: patrick-schultz

Merge Risk: ⚪ Minimal · up to ec3fb

The change preserves aggregate and scan bindings without introducing an actionable merge risk.

Pre-merge checks: 4

Check name	Status	Explanation
Title check		The title clearly identifies the main change: createAgg and createScan preserve the other scope.
Description check		The description explains the bug, the behavior change, the expected result, and the batch impact. It is sufficiently complete for the template because it states that the change does not affect batch.
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.

• 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

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

🟢 Approval recommended

The two targeted changes match the intended scope-preservation behavior, with no unresolved findings.

No open issues 🎉

What changed in this PR

This PR fixes query binding-environment scope creation so creating an aggregation or scan scope no longer clears bindings in the other scope.

Changes:

• createAgg copies evaluation bindings into the aggregation scope while preserving the scan scope.
• createScan copies evaluation bindings into the scan scope while preserving the aggregation scope, matching existing extend behavior.

File	Description
hail/​hail/​src/​is/​hail/​expr/​ir/​Env.scala	Preserves the other scope’s bindings during aggregation and scan scope creation.

_{Review effort: Balanced}

[patrick-schultz]

I was nervous about the change in semantics, so had claude do some digging. Looks like the clearing behavior was introduced in #9041, to fix bugs where environments were merged with incompatible agg/scan environments (one None, one Some). But it looks like this now has no behavior change for any caller, and I agree it is conceptually correct. Good simplification!