Skip to content

Support Different Extractors in Egglog - #761

Closed
saulshanabrook wants to merge 10 commits into
egraphs-good:mainfrom
saulshanabrook:extract-cost-model
Closed

Support Different Extractors in Egglog#761
saulshanabrook wants to merge 10 commits into
egraphs-good:mainfrom
saulshanabrook:extract-cost-model

Conversation

@saulshanabrook

@saulshanabrook saulshanabrook commented Dec 12, 2025

Copy link
Copy Markdown
Member

This PR adds the ability to register different extractors with egglog in rust and then use them when calling (extract expr :extractor <extractor name>). This PR adds the ability to change the extractor from Rust used to during extraction. This can work with the builtin cost of usize but also can support other dynamic costs types.

This removes the need in experimental to register a custom extract command for the set cost cost model, and instead it can just register it and set it as default. (egraphs-good/egglog-experimental#37)

It also removes the need in Python to expose the extractor object and can instead use the normal code path of calling the extract command when we want to use a non standard cost model.

Originally, I just supported registering custom cost models, not arbitrary extractors… However if we want to support dynamic cost types, most of this code was still required, so I thought it made more sense to expose the general way of registering a custom extractor, not just a cost model. Also, this could support other extractor in the future without having to change our API, like if we wanted to hook into a SMT solver.

I know there was some questions if we wanted to settle at this point on whether we want this kind of API or if its too early. I made this PR as a proof of concept to understand the complexity involved, so I won't be offended if we decide not to merge it. I was also inspired to try this now since oliver I believe was blocked on some type checking on the current extract command in experimental. Eventually that could be resolved by reworking our macro system, but this would at least let his code work before then. I originally exposed this in the egglog file as a way to change between registered extractors. I removed that feature, to keep this change more minimal and because I didn't see an immediate need for that flexibility. In Python, we can just change between extractors when users pass them in.

@saulshanabrook
saulshanabrook marked this pull request as ready for review December 12, 2025 03:32
Copilot AI review requested due to automatic review settings December 12, 2025 03:32
@saulshanabrook
saulshanabrook requested a review from a team as a code owner December 12, 2025 03:32
@saulshanabrook
saulshanabrook requested review from oflatt and removed request for a team December 12, 2025 03:32

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 adds support for registering and using different extractors in egglog, enabling dynamic cost models and custom extraction strategies. The implementation introduces a trait-based architecture that allows users to register extractors by name and specify which one to use via the (extract expr :extractor <name>) syntax.

Key changes include:

  • Introduction of DynExtractor and DynExtractorBuilder traits for dynamic extractor dispatch
  • Extension of the Cost trait with clone_box and as_any methods to support dynamic cost types
  • Addition of extractor registration API (register_extractor, set_default_extractor)
  • Parser support for optional :extractor parameter in extract commands
  • Updated test suite with comprehensive coverage of custom cost models and extractors

Reviewed changes

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

Show a summary per file
File Description
src/lib.rs Adds extractor registration infrastructure, dynamic cost type support (DynCost), and CostModelExtractorBuilder for wrapping cost models; replaces direct cost model usage with factory pattern
src/extract.rs Extends Cost trait with object-safe methods (clone_box, as_any) to enable dynamic dispatch; adds Arc<T> implementation for CostModel
src/ast/mod.rs Updates Extract command AST variants to include optional extractor name parameter
src/ast/parse.rs Implements parsing for :extractor <name> option in extract commands with flexible argument ordering
src/ast/desugar.rs Passes through new extractor parameter in Extract command desugaring
src/typechecking.rs Threads extractor name through typechecking without validation (deferred to runtime)
src/cli.rs Moves FromStr import from lib.rs (cleanup)
tests/integration_test.rs Adds helper function for cost downcasting; updates all extract tests to handle dynamic costs; adds comprehensive tests for custom extractors and non-default cost types

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

Comment thread src/lib.rs Outdated
Comment thread src/lib.rs
Comment thread src/lib.rs
Comment thread src/ast/mod.rs Outdated
Comment thread src/ast/parse.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
Comment thread src/lib.rs Outdated
@codspeed-hq

codspeed-hq Bot commented Dec 12, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #761 will not alter performance

Comparing saulshanabrook:extract-cost-model (573ee44) with main (818e4ac)

Summary

✅ 20 untouched
⏩ 190 skipped1

Footnotes

  1. 190 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.

@saulshanabrook

Copy link
Copy Markdown
Member Author

We could also remove the ability to change extractors via the egglog syntax and just store the current extractor on the egraph, and have a way to change it in rust. I think this could make more sense or at least be a smaller change.

@saulshanabrook

Copy link
Copy Markdown
Member Author

Changing this to a draft for now, since I didn't realize it would add this much complexity with the dynamic costs.

@saulshanabrook

Copy link
Copy Markdown
Member Author

Closing for now, because we might want more discussion on the interface first.

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.

2 participants