Skip to content

Add eager merge container primitives#918

Closed
saulshanabrook wants to merge 14 commits into
egraphs-good:mainfrom
saulshanabrook:codex/eager-merge-containers
Closed

Add eager merge container primitives#918
saulshanabrook wants to merge 14 commits into
egraphs-good:mainfrom
saulshanabrook:codex/eager-merge-containers

Conversation

@saulshanabrook

Copy link
Copy Markdown
Member

Stack

Draft PR built on top of #881.

This branch includes the commits from that PR until #881 lands, so review should focus on the final commit, Add eager merge container primitives.

What changed

  • Adds Maybe and Either container sorts with unwrap/match primitives.
  • Adds unstable-if, a Unit-thunk-conditioned primitive that eagerly returns one of two already-built values.
  • Registers unstable-if, Maybe match, and Either match in both declaration orders needed by generated egglog programs.
  • Adds focused .egg tests for Maybe, Either, UnstableFn, and late registration.

Why

eggcc is moving merge functions away from constructor-producing encodings. These primitives let merge bodies compute eagerly over container values, so generated :merge expressions can return an existing value rather than creating a constructor node only to rewrite it later.

Validation

  • cargo build --release
  • cargo build --release --bin egglog-experimental in the dependent egglog-experimental worktree
  • fish -lc 'cargo build --release' in eggcc
  • During development, focused file tests passed for fn, maybe, either, and pair.

@saulshanabrook saulshanabrook changed the title Add eager merge container primitives Add more container primitives Jun 5, 2026
@saulshanabrook saulshanabrook changed the title Add more container primitives Add more container primitives for eager eggcc merge functions Jun 5, 2026
@saulshanabrook saulshanabrook changed the title Add more container primitives for eager eggcc merge functions Add eager merge container primitives Jun 5, 2026
@codspeed-hq

codspeed-hq Bot commented Jun 5, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 7.61%

⚡ 1 improved benchmark
✅ 25 untouched benchmarks
⏩ 216 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation tests[repro-665-set-union] 625.2 ms 581 ms +7.61%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing saulshanabrook:codex/eager-merge-containers (411c991) with main (f0da95b)

Open in CodSpeed

Footnotes

  1. 216 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.67204% with 181 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.57%. Comparing base (57108da) to head (411c991).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
src/exec_state.rs 0.00% 66 Missing ⚠️
src/lib.rs 77.15% 53 Missing ⚠️
src/sort/either.rs 80.87% 35 Missing ⚠️
src/sort/maybe.rs 86.58% 22 Missing ⚠️
src/typechecking.rs 89.36% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #918      +/-   ##
==========================================
- Coverage   87.17%   86.57%   -0.61%     
==========================================
  Files          88       90       +2     
  Lines       26106    27019     +913     
==========================================
+ Hits        22759    23392     +633     
- Misses       3347     3627     +280     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@oflatt

oflatt commented Jun 5, 2026

Copy link
Copy Markdown
Member

Just a note, I think maybe and either seem like egglog-experimental type things right?
I think moving egglog-experimental into this code base would make this a lot easier...

Comment thread src/sort/fn.rs

fn reserved_primitives() -> Vec<&'static str> {
vec!["unstable-fn", "unstable-app"]
vec!["unstable-fn", "unstable-app", "unstable-if"]

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

TODO should remove this extra change here

@saulshanabrook

saulshanabrook commented Jun 6, 2026

Copy link
Copy Markdown
Member Author

Just a note, I think maybe and either seem like egglog-experimental type things right?

Yeah that sounds nice! The issue is that we have to inline certain type registration in the fn.rs registration:

    register_maybe_primitives_for_function(eg, self.clone());
    register_if_primitives_for_function(eg, self.clone());
    register_either_primitives_for_function(eg, self.clone());

To move those to experimental, we could add some kind of thing in core to allow registering callbacks here... I think I could do that, in a way that is non specific to unstable function... Would definately be nice to move a bunch of this stuff to experimental I agree! Also to be able to add particular other function registration to other sorts like Map that are in experimental.

I think moving egglog-experimental into this code base would make this a lot easier...

Yeah once all my in flight PRs (besides this one) land in egglog/experimental I will make a PR to see how that looks!

@saulshanabrook

Copy link
Copy Markdown
Member Author

Closing and replacing with an experimental PR

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