Skip to content

[compiler] StreamAgg{Scan} drops opposite scan/agg env - #15621

Open
ehigham wants to merge 1 commit into
hail-is:mainfrom
ehigham:ehigham/drop-aggscan-env
Open

[compiler] StreamAgg{Scan} drops opposite scan/agg env#15621
ehigham wants to merge 1 commit into
hail-is:mainfrom
ehigham:ehigham/drop-aggscan-env

Conversation

@ehigham

@ehigham ehigham commented Jul 31, 2026

Copy link
Copy Markdown
Member

StreamAgg{Scan} are the only two operations that don't define the
transition on the opposite env. BindingEnv accounted for this by
clearing the other environment when the other had an AggEnv.Create
transition. This feels somewhat ad-hoc and would be better if we
dropped the other environment entirely, given that Extract does
not handle aggs inside scans.

This change does not affect the broad-managed batch service in gcp.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies and regularizes how IR binding environments transition for StreamAgg and StreamAggScan by making them explicitly drop the opposite aggregation environment (scan vs agg), instead of relying on BindingEnv to apply an ad-hoc “clear the other env on Create” rule. This aligns environment behavior with the stated limitation that Extract does not handle aggs inside scans.

Changes:

  • Remove the special-case logic in BindingEnv.modifyWithoutNewBindings that cleared both agg/scan envs whenever either side performed an AggEnv.Create.
  • Update Bindings.get for StreamAgg to explicitly Drop the scan env when creating an agg env.
  • Update Bindings.get for StreamAggScan to explicitly Drop the agg env when creating a scan env.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
hail/hail/src/is/hail/expr/ir/Env.scala Removes the global “clear opposite env on Create” special-case from BindingEnv transitions.
hail/hail/src/is/hail/expr/ir/Binds.scala Makes StreamAgg / StreamAggScan explicitly drop the opposite env (scan / agg) to replace the removed special-case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants