Differential Dataflow Backend#4
Conversation
oflatt-claude
force-pushed
the
backend-interface
branch
2 times, most recently
from
a94e01e to
8c4f4c3
Compare
|
Per review feedback, |
oflatt-claude
force-pushed
the
backend-interface
branch
from
8c4f4c3 to
31334f6
Compare
|
Correction/update: the |
|
Cleanup pass (two commits on top): Tidy egglog-backend-trait docs — fixed stale references ( Simplify the flowlog backend + snapshot harness —
Net: ~1300 fewer lines in the flowlog crate; all suites green (egglog |
oflatt-claude
force-pushed
the
backend-interface
branch
from
e502e35 to
85ffdfb
Compare
|
Runs in ~4s under Worth flagging: those 3 mismatches mean the stripped-down FlowLog is not correct on merge-during-rebuild-style programs — a real limitation of this demo backend, not just "unsupported." Happy to dig into it if it's worth fixing, but I've left it documented rather than silently excluded. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds a pluggable backend interface, integrates an in-process Differential Dataflow backend, updates Egglog rule execution and resource handling, and extends benchmark tooling with backend-aware collection, Markdown reports, profiling workflows, and Samply CPU analysis. ChangesBackend platform and frontend integration
Differential-dataflow backend
Backend-aware benchmarking and profiling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant EgglogFrontend
participant BackendRule
participant FusedDdJoin
participant DdEGraph
EgglogFrontend->>BackendRule: compile RuleSpec
BackendRule->>FusedDdJoin: register rule join plan
DdEGraph->>FusedDdJoin: submit relation deltas
FusedDdJoin-->>DdEGraph: return binding deltas
DdEGraph->>EgglogFrontend: apply staged rule updates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
Add a backend-agnostic SPI so a third party can drive the egglog frontend
with their own execution engine, and ship a differential-dataflow ("FlowLog")
backend as the worked example.
Backend SPI (new `egglog-backend-trait` crate):
- `egglog::EGraph` holds a `Box<dyn Backend>` and performs all state access
through the `Backend` trait; `EGraph::with_backend(..)` plugs in a custom one.
- Rules are built through `RuleBuilderOps`, mirroring `egglog_bridge::RuleBuilder`.
- A backend without a native union-find declares `requires_term_encoding()`;
`EGraph::with_term_encoding()` opts such an e-graph into the term-encoding
pipeline (congruence and rebuild lower to rules over `@uf` tables), and running
one without it errors (`Error::BackendRequiresTermEncoding`) rather than
silently dropping `union`s.
FlowLog backend (`egglog-experimental/flowlog`):
- Runs rule bodies on an in-process differential-dataflow join; body primitives
and head actions apply host-side into a Rust mirror.
- Correct under term encoding: congruence closure folds merge-function sets in
insertion order (a sort-based fold left the `@UF_Mf` find-index stale);
computed merges evaluate the retained merge tree (primitives like `(or old
new)`, constructor merges like `(C2 (C1 old new) ...)`); subsumption is a soft
delete (subsumed rows move to a side set, hidden from ordinary matching but
visible to `for_each` and the include-subsumed rebuild rules).
- A corpus harness (`tests/files.rs`) runs egglog's `.egg` corpus on FlowLog vs a
term-encoded reference for cross-backend parity + insta snapshots; only
genuinely-unsupported files (push/pop) are excluded.
Both new crates join the root workspace, so `cargo test --workspace` and
`clippy --workspace` cover them. The FlowLog `[patch.crates-io]` pinning
differential-dataflow lives in the workspace-root Cargo.toml.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
oflatt-claude
force-pushed
the
backend-interface
branch
from
aecfcf6 to
efef7a7
Compare
A primitive registered through the Rust API (`add_pure_primitive` / `add_*_primitive` / the `add_primitive!` macros) after e-graph construction was only registered on the running e-graph, not on the separate `original_typechecking` e-graph that term encoding / proofs use to typecheck the encoded program. The encoder then reported the primitive as an unbound function, so callers had to manually register it on `proof_state.original_typechecking` as well (a proof test did exactly that). `register_per_context` now walks the `original_typechecking` chain and registers the primitive on each e-graph. A typechecker only typechecks and never evaluates, so the wrapper's runtime state is irrelevant there, and both e-graphs register the same built-ins at construction, so a primitive added to both gets the same `ExternalFunctionId`. A sort's primitives already reach the typechecker through its own `add_arcsort` when the typechecker typechecks the sort command, so `add_arcsort` detaches the typechecker while a sort registers to avoid double-registering (which would make primitive resolution ambiguous). Upstreamed as egraphs-good/egglog#945. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
oflatt-claude
force-pushed
the
backend-interface
branch
from
e6335d6 to
6a8f576
Compare
egglog's `tests/files.rs` appends `(print-size)` to every corpus file and stores one snapshot per file whose content is normalized to be identical across the native / term-encoding / proofs treatments (`CommandOutput::snapshot_stable_under_proof_encoding`). FlowLog is term-encoded, so its normalized output must equal that snapshot. The FlowLog corpus harness now checks against those shared snapshots directly (`egglog/tests/snapshots/files__shared_snapshot_<stem>.snap`) instead of an in-process reference plus its own snapshots — egglog is the single source of truth, and FlowLog's own snapshots are dropped (as is the `insta` dev-dep). Files are compared only when `file_supports_proofs` (egglog ran the term-encoding treatment, so the snapshot is term-encoding-valid); native-only snapshots are skipped. Comparing the FULL normalized output (not just the last `print-size`) is stricter and surfaced two real FlowLog divergences the old check missed — `antiunify` and `integer_math` leave a few extra un-collapsed constructor e-nodes (incomplete congruence closure). They are documented in `KNOWN_MISMATCH` for follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Handoff comment from @oflatt Handoff — flowlog backend + egglog term-encoding fix Two PRs, both green (except one documented divergence): - #4 (#4) (branch backend-interface, pushed to oflatt-claude fork): pluggable Backend SPI + FlowLog backend, rebased on current main, both crates are root-workspace members (in cargo test/clippy --workspace). - egraphs-good/egglog#945 (egraphs-good/egglog#945) (upstream): fix so Rust-API primitives register on the term-encoding typechecker (was "Unbound function"). Full upstream suite green. Just landed: FlowLog's tests/files.rs now checks against egglog's own golden snapshots (egglog/tests/snapshots/files__shared_snapshot_*.snap) — egglog is the single source of truth. Files are compared only when file_supports_proofs (snapshot is term-encoding-valid); FlowLog's own snapshots + insta dep removed. |
| /// leading `@` and trailing digits don't matter. Order: most specific first | ||
| /// (`singleparent` and the `_subsume` variant before their bare forms; | ||
| /// `uf_function_index` before `uf_update`). | ||
| pub(crate) fn rule_category(name: &str) -> &'static str { |
There was a problem hiding this comment.
red flag- special casing rule sets
There was a problem hiding this comment.
Perhaps the problem here is more with the backend trait not preserving the ruleset name? Then we could remove this if we fixed that?
There was a problem hiding this comment.
Added RuleSetRun which carries the name through
saulshanabrook
changed the title
Resolve main conflicts while keeping the FlowLog DD backend experimental, remove prototype WCOJ/env-toggle paths, and document current unsupported FlowLog parity boundaries.
|
I have addressed some of the comments and we can see how its a bit slower than current egglog on all files test. I will now work on profiling to see if there is any low hanging fruit around fixing these: $ ./bench.py --backend main,flowlog --treatments proofs egglog/tests/web-demo/matrix.egg egglog/tests/web-demo/resolution.egg egglog/tests/integer_math.egg egglog/tests/web-demo/rw-analysis.egg --format markdownBenchmark Report
Targets
ComparisonsPer-file backend wall-time change vs main: 230bf52 flowlog
Ratio is candidate backend / baseline backend for the same target and treatment. Values below 1x are faster; above 1x are slower. Intervals are 95% CIs. Per-file backend peak RSS change vs main: 230bf52 flowlog
Ratio is candidate backend / baseline backend for the same target and treatment. Values below 1x use less peak RSS; above 1x use more. Intervals are 95% CIs. Target Diagnostics230bf52: per-file wall time
Within-target wall-time estimates. These are not target-vs-baseline ratios. 230bf52: per-file peak RSS
Within-target peak resident set size estimates. These are separate from wall-time ratios. Benchmark SummaryFlowLog vs main wall time
Ratio is candidate backend / baseline backend for the same target and treatment. Values below 1x are faster; above 1x are slower. Intervals are 95% CIs. FlowLog vs main peak RSS
Ratio is candidate backend / baseline backend for the same target and treatment. Values below 1x use less peak RSS; above 1x use more. Intervals are 95% CIs. 230bf52: proof overhead summary
Within-backend proof overhead. This is not backend-vs-main performance. |
|
This is now ready for review. It is about 4x slower than main on a reduced math microbenchmark, which is a lot better than before. I think we should get this in for now, and keep iterating, either by replacing this with duckdb or doing a larger refactor to make it faster. I think we should merge this in for now, to get the code that allows benchmarking against multiple backends, I also update the benchmark script to output markdown (helpful for posting to github), and to easily profile. I have also renamed it to a Here is an update from the agent on what further performance improvements would take:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
egglog/src/proofs/proof_extractor.rs (1)
176-194: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against panics when function tables are empty.
Both
first_outputandfirst_inputcallvalue.unwrap()after iterating withfor_each_while. If the function's backend table has no rows,valueremainsNoneand theunwrap()panics. With the new pluggable backend SPI, an alternate backend that hasn't populated rows yet (or a query that matches nothing) will trigger a runtime panic instead of a graceful error.Consider returning a
Result<Value, Error>or providing a descriptiveexpect()message that includes the function name for debuggability.🛡️ Suggested improvement for `first_output`
fn first_output(egraph: &EGraph, function_name: &str) -> Value { let func = egraph.functions.get(function_name).unwrap(); let mut value = None; egraph.backend.for_each_while(func.backend_id, |row| { value = Some(row.vals[func.extraction_output_index()]); false }); - value.unwrap() + value.unwrap_or_else(|| panic!("no rows in function '{function_name}'")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/src/proofs/proof_extractor.rs` around lines 176 - 194, Update first_output and first_input to handle empty backend tables without an uninformative unwrap panic. Prefer propagating a Result<Value, Error> through their callers; otherwise replace each unwrap with a descriptive expect message that includes function_name, while preserving the existing first-row extraction behavior.egglog/src/lib.rs (1)
1214-1271: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRule is built in the backend before the target ruleset is validated — leaks the rule on
NoSuchRuleset/CombinedRulesetError.
compiled(which callsself.backend.new_rule(...)andtranslator.build(), allocating/registering a backend-side rule) is computed unconditionally, before checking whetherrule.rulesetexists and is aRuleset::Rules(notCombined). If either check fails, the function returns anErrwithout ever callingself.backend.free_rule(rule_id)on the rule that was just built — the backend resource is silently leaked. This is reachable via a plain user typo (referencing an undeclared or combined ruleset in:ruleset), and other call sites in this same file (eval_actions,check_facts,query) are careful to free rules they build; this path isn't.🔧 Proposed fix: validate the ruleset before compiling
let union_to_set = self.proof_state.original_typechecking.is_none(); + match self.rulesets.get(&rule.ruleset) { + Some(Ruleset::Rules(_)) => {} + Some(Ruleset::Combined(_)) => { + return Err(Error::CombinedRulesetError(rule.ruleset, rule.span)); + } + None => return Err(Error::NoSuchRuleset(rule.ruleset, rule.span)), + } + let compiled: Vec<(String, core::ResolvedCoreRule, egglog_bridge::RuleId)> = { ... }; - if let Some(rules) = self.rulesets.get_mut(&rule.ruleset) { - match rules { - Ruleset::Rules(rules) => { - for (name, core_rule, rule_id) in compiled { - match rules.entry(name.clone()) { - indexmap::map::Entry::Occupied(_) => { - panic!("Rule '{name}' was already present") - } - indexmap::map::Entry::Vacant(e) => e.insert((core_rule, rule_id)), - }; - } - Ok(rule.name) - } - Ruleset::Combined(_) => Err(Error::CombinedRulesetError(rule.ruleset, rule.span)), - } - } else { - Err(Error::NoSuchRuleset(rule.ruleset, rule.span)) - } + let Some(Ruleset::Rules(rules)) = self.rulesets.get_mut(&rule.ruleset) else { + unreachable!("ruleset validated above") + }; + for (name, core_rule, rule_id) in compiled { + match rules.entry(name.clone()) { + indexmap::map::Entry::Occupied(_) => panic!("Rule '{name}' was already present"), + indexmap::map::Entry::Vacant(e) => e.insert((core_rule, rule_id)), + }; + } + Ok(rule.name) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/src/lib.rs` around lines 1214 - 1271, Validate that rule.ruleset exists and is a Ruleset::Rules before constructing compiled in add_rule. Move the self.rulesets lookup and Combined/NoSuchRuleset error handling ahead of self.backend.new_rule and translator.build, then compile and insert only after validation succeeds, preserving duplicate-name handling.
🧹 Nitpick comments (6)
egglog-experimental/Cargo.toml (1)
21-28: 🧹 Nitpick | 🔵 Trivial
dd-backendis pulled intodefaultviabin, adding DD/Timely to default builds.Since
default = ["bin"]andbinnow includesdd-backend, every consumer building this crate with default features (including pure library users) compiles inegglog-experimental-ddand its Timely/Differential-Dataflow dependency tree, even if they never use the DD backend. Given DD is explicitly experimental (slower, higher memory, known correctness gaps per PR notes), consider whethermain.rs's reliance on it could be cfg-gated sodd-backendis opt-in rather than bundled into the default feature set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog-experimental/Cargo.toml` around lines 21 - 28, Remove dd-backend from the default bin feature set so default builds do not enable egglog-experimental-dd or its Timely/Differential-Dataflow dependencies. Keep the dd-backend feature available as an explicit opt-in, and cfg-gate the main.rs DD backend integration and related binary behavior behind that feature.egglog/egglog-backend-trait/src/lib.rs (2)
384-397: 📐 Maintainability & Code Quality | 🔵 TrivialHardcoded
minmerge for container registration on non-bridge backends.Line 390 hardcodes
std::cmp::min(old, new)as the merge function for all container types on non-bridge backends. If different container sorts require different merge semantics, this will produce incorrect results. Consider whether this should be configurable per container type.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/egglog-backend-trait/src/lib.rs` around lines 384 - 397, Update register_container_ty so non-bridge backends obtain and use merge semantics configurable for each container type instead of always applying std::cmp::min to old and new values. Preserve the existing bridge registration path and container registry validation, and route the selected merge function through the container registration API.
253-267: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winDefault
supports_action_registry()tofalse. Backends that actually expose anActionRegistrycan opt in explicitly; leaving thistruemakes theaction_registry()panic reachable if a backend misses the override.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/egglog-backend-trait/src/lib.rs` around lines 253 - 267, Change the default implementation of Backend::supports_action_registry() to return false, while leaving action_registry() and backend-specific opt-in overrides unchanged. Backends that expose an ActionRegistry should continue to override supports_action_registry() explicitly.egglog/src/proofs/proof_extraction.rs (1)
87-108: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winProof-value lookup still does a linear scan.
for_each_whilewalks the proof table row by row here, and the backend’s keyedlookupAPI is RHS-only, so it can’t replace this read path. If this is on a hot path, a read-only keyed accessor would remove the per-call scan.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/src/proofs/proof_extraction.rs` around lines 87 - 108, Replace the linear proof-table scan in the proof-value lookup block with a read-only keyed accessor that retrieves the row associated with witness_value directly. Add or reuse a backend API that supports lookup by the proof table’s left-hand key while preserving the existing behavior of returning the second value or None; leave proof-function resolution unchanged.egglog/src/proofs/proof_checker.rs (1)
23-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNew container-var-equality branch lacks test coverage.
The added
is_container_var(lhs) && is_container_var(rhs)branch (two container-sorted variables equated without a container-producing primitive on either side) is a genuinely new code path. The existing regression test (proof_mode_allows_pair_container_side_conditionsinproof_tests.rs) only exercises the pre-existing container-primitive case ((= c (pair e 1))), not this new var/var case. Since containers have no UF proof rows, a mismatch between what the encoder classifies as a side condition and what the checker re-validates would fail silently or error confusingly — worth a dedicated test.Could you add a case where two already-bound container-sorted variables (or one bound/one produced only via variable propagation, not directly from a primitive call) are compared for equality in a rule body, to exercise this new branch end-to-end?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/src/proofs/proof_checker.rs` around lines 23 - 50, Add a dedicated proof regression test covering the is_container_side_condition branch where an Eq compares two container-sorted ResolvedExpr::Var values with no container-producing primitive on either side. Build a rule body using two already-bound container variables (or variable propagation), assert their equality, and verify proof checking succeeds end-to-end alongside proof_mode_allows_pair_container_side_conditions.egglog/src/lib.rs (1)
1488-1494: 🩺 Stability & Availability | 🔵 TrivialBridge-only capability mismatches panic via
.expect()instead of returning a recoverableError.
unstable-fnresolution (here and inBackendRule::prim) andinput_file's fact loading all hard-panic when the backend isn'tegglog_bridge::EGraph, whereas this same PR introduces a gracefulError::BackendRequiresTermEncodingfor a different backend-capability mismatch (Line 2215-2218). For a library whose whole point is now a pluggable backend SPI, an embedder using a custom backend that hits(input ...)orunstable-fngets an unrecoverable panic instead of a catchable error. This is likely an acceptable interim tradeoff for these currently bridge-only features, but worth tracking as the backend SPI matures — panics here will crash any host application, not just the egglog CLI.Also applies to: 2020-2025, 2765-2769
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/src/lib.rs` around lines 1488 - 1494, Replace the bridge downcast `.expect()` calls in unstable-function resolution, `BackendRule::prim`, and `input_file` fact loading with recoverable backend-capability errors. Propagate an appropriate `Error` when the backend is not `egglog_bridge::EGraph`, preserving normal behavior for the reference bridge backend and avoiding any panic for custom backends.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@egglog-experimental/dd/src/interpret.rs`:
- Around line 218-260: Replace the release-disabled debug_assert_eq! in the
fused join mapping block with an unconditional validation, or derive caller
positions by looking up each value from fused.rule_indices() in atom_rule_idxs
rather than assuming positional equality with atom_positions. Ensure cached
FusedDdJoin entries cannot silently map DD outputs to the wrong caller rule, and
preserve fail-fast behavior on any ordering mismatch.
In `@egglog/src/proofs/proof_encoding.rs`:
- Around line 950-972: Remove the local is_container_primitive,
is_container_var, and is_container_side_condition helpers in the proof encoding
logic, and reuse the corresponding shared pub(super) predicate from
proof_checker.rs. Update the call sites to reference that shared implementation
so encoding and verification classify container side conditions identically.
In `@egglog/src/scheduler.rs`:
- Around line 237-242: Update step_rules_with_scheduler and its
with_execution_state closure to avoid the expect on the downcast to
egglog_bridge::EGraph. Hoist the bridge conversion outside the per-rule
iteration where practical, convert a non-bridge backend failure into
Error::BackendError, and propagate it through the existing Result<RunReport,
Error> flow.
---
Outside diff comments:
In `@egglog/src/lib.rs`:
- Around line 1214-1271: Validate that rule.ruleset exists and is a
Ruleset::Rules before constructing compiled in add_rule. Move the self.rulesets
lookup and Combined/NoSuchRuleset error handling ahead of self.backend.new_rule
and translator.build, then compile and insert only after validation succeeds,
preserving duplicate-name handling.
In `@egglog/src/proofs/proof_extractor.rs`:
- Around line 176-194: Update first_output and first_input to handle empty
backend tables without an uninformative unwrap panic. Prefer propagating a
Result<Value, Error> through their callers; otherwise replace each unwrap with a
descriptive expect message that includes function_name, while preserving the
existing first-row extraction behavior.
---
Nitpick comments:
In `@egglog-experimental/Cargo.toml`:
- Around line 21-28: Remove dd-backend from the default bin feature set so
default builds do not enable egglog-experimental-dd or its
Timely/Differential-Dataflow dependencies. Keep the dd-backend feature available
as an explicit opt-in, and cfg-gate the main.rs DD backend integration and
related binary behavior behind that feature.
In `@egglog/egglog-backend-trait/src/lib.rs`:
- Around line 384-397: Update register_container_ty so non-bridge backends
obtain and use merge semantics configurable for each container type instead of
always applying std::cmp::min to old and new values. Preserve the existing
bridge registration path and container registry validation, and route the
selected merge function through the container registration API.
- Around line 253-267: Change the default implementation of
Backend::supports_action_registry() to return false, while leaving
action_registry() and backend-specific opt-in overrides unchanged. Backends that
expose an ActionRegistry should continue to override supports_action_registry()
explicitly.
In `@egglog/src/lib.rs`:
- Around line 1488-1494: Replace the bridge downcast `.expect()` calls in
unstable-function resolution, `BackendRule::prim`, and `input_file` fact loading
with recoverable backend-capability errors. Propagate an appropriate `Error`
when the backend is not `egglog_bridge::EGraph`, preserving normal behavior for
the reference bridge backend and avoiding any panic for custom backends.
In `@egglog/src/proofs/proof_checker.rs`:
- Around line 23-50: Add a dedicated proof regression test covering the
is_container_side_condition branch where an Eq compares two container-sorted
ResolvedExpr::Var values with no container-producing primitive on either side.
Build a rule body using two already-bound container variables (or variable
propagation), assert their equality, and verify proof checking succeeds
end-to-end alongside proof_mode_allows_pair_container_side_conditions.
In `@egglog/src/proofs/proof_extraction.rs`:
- Around line 87-108: Replace the linear proof-table scan in the proof-value
lookup block with a read-only keyed accessor that retrieves the row associated
with witness_value directly. Add or reuse a backend API that supports lookup by
the proof table’s left-hand key while preserving the existing behavior of
returning the second value or None; leave proof-function resolution unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a970c91-5347-4804-a2d6-4b36fe195b3d
⛔ Files ignored due to path filters (5)
Cargo.lockis excluded by!**/*.lockegglog-experimental/tests/fixtures/eggcc-2mm-pass1-merge-old.eggis excluded by!**/*.eggegglog/tests/math-microbenchmark-mini.eggis excluded by!**/*.eggegglog/tests/snapshots/files__proof_unsupported_files.snapis excluded by!**/*.snapegglog/tests/snapshots/files__shared_snapshot_math_microbenchmark_mini.snapis excluded by!**/*.snap
📒 Files selected for processing (42)
.gitignoreCargo.tomlbench.pyegglog-experimental/Cargo.tomlegglog-experimental/dd/.gitignoreegglog-experimental/dd/Cargo.tomlegglog-experimental/dd/src/compile.rsegglog-experimental/dd/src/dd_native.rsegglog-experimental/dd/src/external_func.rsegglog-experimental/dd/src/interpret.rsegglog-experimental/dd/src/lib.rsegglog-experimental/dd/src/rule_builder.rsegglog-experimental/dd/tests/files.rsegglog-experimental/dd/tests/rewrite_join.rsegglog-experimental/dd/tests/smoke.rsegglog-experimental/src/lib.rsegglog-experimental/src/main.rsegglog/CHANGELOG.mdegglog/Cargo.tomlegglog/egglog-backend-trait/Cargo.tomlegglog/egglog-backend-trait/src/backend_impl.rsegglog/egglog-backend-trait/src/lib.rsegglog/egglog-bridge/src/lib.rsegglog/src/cli.rsegglog/src/lib.rsegglog/src/prelude.rsegglog/src/proofs/proof_checker.rsegglog/src/proofs/proof_encoding.rsegglog/src/proofs/proof_encoding_helpers.rsegglog/src/proofs/proof_extraction.rsegglog/src/proofs/proof_extractor.rsegglog/src/proofs/proof_simplification.rsegglog/src/proofs/proof_tests.rsegglog/src/scheduler.rsegglog/src/serialize.rsegglog/src/sort/fn.rsegglog/src/sort/mod.rsegglog/src/sort/pair.rsegglog/src/sort/set.rsegglog/src/typechecking.rssamply_analysis.pytest_bench.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@egglog/src/scheduler.rs`:
- Line 378: Make rule construction transactional across qrule_builder.query and
the later query-rule build: if either operation fails, release the previously
registered external function, decided table, and query rule handles before
propagating the error. Update the surrounding rule-construction flow so
successful construction retains all registrations, while any failure cleans up
every resource allocated during that attempt.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f7c126e-25f3-4e16-98c0-133216f784c2
📒 Files selected for processing (15)
bench.pyegglog-experimental/Cargo.tomlegglog-experimental/dd/src/interpret.rsegglog-experimental/dd/src/lib.rsegglog-experimental/dd/tests/smoke.rsegglog-experimental/src/main.rsegglog/egglog-backend-trait/src/backend_impl.rsegglog/egglog-backend-trait/src/lib.rsegglog/src/lib.rsegglog/src/proofs/proof_encoding.rsegglog/src/proofs/proof_extraction.rsegglog/src/proofs/proof_extractor.rsegglog/src/proofs/proof_tests.rsegglog/src/scheduler.rstest_bench.py
🚧 Files skipped from review as they are similar to previous changes (12)
- egglog/src/proofs/proof_extractor.rs
- egglog/src/proofs/proof_tests.rs
- egglog/egglog-backend-trait/src/backend_impl.rs
- egglog-experimental/dd/tests/smoke.rs
- egglog-experimental/src/main.rs
- egglog/egglog-backend-trait/src/lib.rs
- egglog-experimental/dd/src/interpret.rs
- test_bench.py
- egglog-experimental/dd/src/lib.rs
- egglog/src/lib.rs
- egglog/src/proofs/proof_encoding.rs
- bench.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
egglog/src/lib.rs (1)
2807-2831: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
unstable-fnshould propagate resolution failures instead of panicking
This branch turns a recoverable lookup error into a panic. To match the otherunstable-fnpaths, return theErrhere and add a way to release the pre-registeredpanic_idfirst;RuleBuilderOpsdoesn’t expose that cleanup hook today.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/src/lib.rs` around lines 2807 - 2831, The unstable-fn resolution path currently panics on errors instead of returning them. Update resolve_function_container_target_with_context handling to propagate its Err result, and extend RuleBuilderOps with the cleanup operation needed to release the pre-registered panic_id before returning the failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@egglog/egglog-bridge/src/lib.rs`:
- Around line 426-469: Add tests covering both failure paths in
remove_last_table: unknown or non-last FunctionId requests must return an error
without altering registrations, and a failed db.remove_last_table(info.table)
must return an error while restoring the removed function through
self.funcs.insert. Verify the relevant function/table state remains intact after
each failure, alongside the existing success-path test.
---
Outside diff comments:
In `@egglog/src/lib.rs`:
- Around line 2807-2831: The unstable-fn resolution path currently panics on
errors instead of returning them. Update
resolve_function_container_target_with_context handling to propagate its Err
result, and extend RuleBuilderOps with the cleanup operation needed to release
the pre-registered panic_id before returning the failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fa8838a-fabf-4b85-ab97-1bd290d9d3bc
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.tomlbench.pyegglog-experimental/dd/Cargo.tomlegglog-experimental/dd/src/compile.rsegglog-experimental/dd/src/dd_native.rsegglog-experimental/dd/src/interpret.rsegglog-experimental/dd/src/lib.rsegglog-experimental/dd/src/rule_builder.rsegglog-experimental/src/main.rsegglog/core-relations/src/dependency_graph.rsegglog/core-relations/src/free_join/mod.rsegglog/egglog-backend-trait/Cargo.tomlegglog/egglog-backend-trait/src/backend_impl.rsegglog/egglog-backend-trait/src/lib.rsegglog/egglog-bridge/src/lib.rsegglog/egglog-bridge/src/rule.rsegglog/egglog-bridge/src/tests.rsegglog/numeric-id/src/lib.rsegglog/numeric-id/src/tests.rsegglog/src/lib.rsegglog/src/proofs/proof_encoding.rsegglog/src/proofs/proof_extraction.rsegglog/src/scheduler.rsegglog/src/typechecking.rstest_bench.py
💤 Files with no reviewable changes (1)
- egglog/egglog-backend-trait/Cargo.toml
oflatt
left a comment
oflatt
left a comment
There was a problem hiding this comment.
We're getting close! Some scattered feedback, hopefully we can get a clean diff and not touch proof checker / proof encoding
| /// leading `@` and trailing digits don't matter. Order: most specific first | ||
| /// (`singleparent` and the `_subsume` variant before their bare forms; | ||
| /// `uf_function_index` before `uf_update`). | ||
| pub(crate) fn rule_category(name: &str) -> &'static str { |
There was a problem hiding this comment.
Perhaps the problem here is more with the backend trait not preserving the ruleset name? Then we could remove this if we fixed that?
|
|
||
| /// The stable content of egglog's shared snapshot for `stem`, if egglog produced | ||
| /// one. Strips insta's `---`-delimited YAML header. | ||
| fn egglog_snapshot(stem: &str) -> Option<String> { |
There was a problem hiding this comment.
perhaps instead of reading the snapshot and doing string mangling, we could use insta the normal way? That probably works even if the files are in another dir
| use crate::EGraph; | ||
|
|
||
| /// Accumulates a rule's body ops and head ops, then registers them. | ||
| pub struct FlowlogRuleBuilder<'a> { |
There was a problem hiding this comment.
Optional: I wonder if we should reconsider the rule builder API we inherited from the old backend. If flowlog has to construct a AST anyways, perhaps we should just send the AST to the backend? The existing backend could use a rule builder internally if it wants to.
There was a problem hiding this comment.
Moves generic rules to egglog-ast and sent in rule spec to backends
| /// (set-of b))`, etc. Its proof is a bare [`Justification::Eval`] marker and it | ||
| /// is verified by re-evaluation in [`ProofStore::check_side_condition`] rather | ||
| /// than by matching a premise proposition. | ||
| /// A container side condition is a rule-body fact whose equality is over |
There was a problem hiding this comment.
Did you mean to change the proof checker? I thought the change to container side conditions was not necessary after all?
| pub uf_function: HashMap<String, String>, | ||
| /// Maps sort name -> proof function name (set from :internal-proof-func annotation). | ||
| pub proof_func_parent: HashMap<String, String>, | ||
| /// Function name -> (hidden current-value function, input arity). The |
There was a problem hiding this comment.
What are these proof encoding changes? Perhaps the diff got messed up or something with an old PR?
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
egglog/egglog-bridge/src/rule.rs (1)
207-214: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
new_panic/panicdon't track their allocated external function for cleanup, unlikenew_panic_lazy.
new_panic_lazypushes its allocated id intoself.resources.external_funcsso it gets freed viaowned_external_funcswhen the rule is freed or rolled back.new_panic(line 208) andpanic(line 739) instead callself.resources.egraph.new_panic(...)directly and never record the id anywhere. SinceEGraph::new_panicincrements a message-keyed reference count on every call, each call through these twoRuleBuildermethods permanently bumps that count with no corresponding decrement when the rule is later freed viaEGraph::free_rule/rollback — undermining the reference-counted cleanup this diff otherwise establishes (bounded leak, growing with the set of distinct messages passed to.panic()/.new_panic()across freed rules).🩹 Suggested fix
pub fn new_panic(&mut self, message: String) -> crate::ExternalFunctionId { - self.resources.egraph.new_panic(message) + let panic = self.resources.egraph.new_panic(message); + self.resources.external_funcs.push(panic); + panic }pub fn panic(&mut self, message: String) { - let panic = self.resources.egraph.new_panic(message.clone()); + let panic = self.new_panic(message.clone()); let ret_ty = ColumnTy::Id;Also applies to: 737-747
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/egglog-bridge/src/rule.rs` around lines 207 - 214, Update RuleBuilder::new_panic and RuleBuilder::panic to record each returned ExternalFunctionId in self.resources.external_funcs, matching new_panic_lazy. Preserve returning the allocated id while ensuring owned_external_funcs can release these registrations during rule cleanup or rollback.egglog/src/typechecking.rs (1)
329-357: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid the raw
panic!()fallback for registry-free backends.register_registry_primitivestill installs a panic-on-call stub whensupports_action_registry()is false, and nothing guardsadd_read_primitive/add_write_primitive/add_full_primitivefrom being used on such a backend. That turns a reachable primitive call into a process abort instead of a controlled error. Route this through the backend’s deferred-panic path or anotherResult-based failure channel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@egglog/src/typechecking.rs` around lines 329 - 357, Update register_registry_primitive so registry-free backends do not install a raw panic-on-call stub. Route attempts to invoke the unsupported primitive through the backend’s existing deferred-panic mechanism or another Result-based error path, and ensure add_read_primitive, add_write_primitive, and add_full_primitive remain controlled failures rather than process aborts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@egglog-experimental/dd/src/compile.rs`:
- Around line 7-8: Align compile::MAX_ARITY with dd_native::W so add_table
rejects any arity unsupported by fixed-width join rows, including arities 49–64;
derive one constant from the other if practical. In
egglog-experimental/dd/src/dd_native.rs lines 184-185, retain the existing
oversized-atom check and ensure validation occurs before plan_join can receive
an admitted unsupported relation; no direct change is required there if the
MAX_ARITY fix guarantees this ordering.
---
Outside diff comments:
In `@egglog/egglog-bridge/src/rule.rs`:
- Around line 207-214: Update RuleBuilder::new_panic and RuleBuilder::panic to
record each returned ExternalFunctionId in self.resources.external_funcs,
matching new_panic_lazy. Preserve returning the allocated id while ensuring
owned_external_funcs can release these registrations during rule cleanup or
rollback.
In `@egglog/src/typechecking.rs`:
- Around line 329-357: Update register_registry_primitive so registry-free
backends do not install a raw panic-on-call stub. Route attempts to invoke the
unsupported primitive through the backend’s existing deferred-panic mechanism or
another Result-based error path, and ensure add_read_primitive,
add_write_primitive, and add_full_primitive remain controlled failures rather
than process aborts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6bc351c7-5498-4072-935c-9beabc0a696a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
egglog-experimental/dd/Cargo.tomlegglog-experimental/dd/src/compile.rsegglog-experimental/dd/src/dd_native.rsegglog-experimental/dd/src/interpret.rsegglog-experimental/dd/src/lib.rsegglog-experimental/dd/tests/files.rsegglog/egglog-ast/src/core.rsegglog/egglog-ast/src/lib.rsegglog/egglog-backend-trait/Cargo.tomlegglog/egglog-backend-trait/src/backend_impl.rsegglog/egglog-backend-trait/src/lib.rsegglog/egglog-bridge/src/lib.rsegglog/egglog-bridge/src/rule.rsegglog/egglog-bridge/src/tests.rsegglog/src/constraint.rsegglog/src/core.rsegglog/src/lib.rsegglog/src/proofs/proof_tests.rsegglog/src/scheduler.rsegglog/src/typechecking.rsegglog/tests/typed_primitive.rs
💤 Files with no reviewable changes (1)
- egglog/src/proofs/proof_tests.rs
3 participants
Summary
This PR adds a pluggable backend SPI to egglog and an experimental backend implemented with Differential Dataflow (DD).
The default remains egglog's reference in-memory backend. The DD backend is a research prototype for testing whether the frontend, term encoding, and proof mode can run over a separately implemented relational backend. It is not proposed as the default backend, and it does not currently outperform the main backend.
The PR also extends the benchmark runner so the two backends can be compared and CPU-profiled with the same workload construction.
Backend boundary
egglog-backend-traitcrate and makes the frontend own aBox<dyn Backend>.EGraph::with_backend(...)for supplying another implementation.egglog-bridgeto the same trait.RuleSpec, built from generic core rule types inegglog-ast, instead of exposing frontend-only resolved types or adding a DD-specific rule IR.Differential Dataflow backend
The experimental backend lives in
egglog-experimental/ddand depends on the public backend SPI, the neutral shared rule AST, and core-relations value/primitive support.Its architecture is deliberately hybrid:
Databaseowns base values, container values, and ordinary primitive execution.The join planner uses a fixed-width row with liveness-based column allocation. It reuses columns across wide variable chains, but rejects a rule whose simultaneously live variable frontier exceeds 48. Table registration, row packing, and rule validation share that same width bound. Cached fused outputs are mapped back to caller rules by stable rule ID, including when callers reverse rule order.
Current semantic coverage
old,new, union-id/min, and computed primitive/function/constant merge trees.clone_boxedby cloning authoritative relation, rule, mirror, value-registry, and version state while rebuilding transient Timely workers lazily. Frontend push/pop is covered end-to-end.The release DD harness runs 129 trials: 127 selected corpus cases plus two corpus-discovery self-checks. Four selected files are expected to return their exact documented action-registry capability error, and all other selected cases must match egglog's shared snapshots. Panics, wrong errors, unexpected failures, stale unsupported entries, unreadable discovery, and empty discovery all fail the harness.
Known limitations
ActionRegistry. Registry-backed container rebuild/read primitives therefore cannot read the authoritative DD mirror. The explicit expected-error cases arecontainer-proofs,datatypes,nested-container-dirty-propagation, andrepro-querybug3.eggcc-2mm,eqsolve,math-microbenchmark,rectangle,repro-herbie-vanilla,resolution, anduntil.Benchmarking and profiling
Benchmark either or both backends while recording the backend in every JSON row:
Reports include per-file and aggregate backend comparisons, best-case visibility, Rich and GitHub Markdown output, append-only result caching, and backend/treatment compatibility checks.
Samply profiles use the same workload construction and a key-addressed local cache:
Validation on
47c469c7cargo test --workspacepasses, including all 712 core file tests and DD's 24 unit, 8 debug-corpus, 2 rewrite, and 7 smoke tests.cargo test --release -p egglog-experimental-dd --test filespasses all 129 trials.cargo clippy --workspace --all-targets -- -D warningspasses.egglog-backend-traitandegglog-experimental-dd. Strict workspace rustdoc remains blocked by two pre-existingorigin/mainlink warnings outside this PR's changed documentation.uv lock --check, Ruff formatting/linting, mypy, and all 96 Python tests pass.efd5b703onmath-microbenchmark-minifound no measurable cleanup regression: wall-time CI[-20.4%, +19.5%], RSS CI[-0.9%, +1.3%].git diff --checkandcargo fmt --all -- --checkpass.proof_encoding.rsandproof_checker.rshave no diff frommain; unrelated proof-encoding edits were removed.Suggested review order
egglog/egglog-backend-traitand the backend-neutral rule types.egglog-experimental/ddstate model and host/DD execution split.The main design question is whether this backend trait and hybrid DD/host implementation are a useful experimental baseline for future backends, given the explicit capability and performance limits above.
Summary by CodeRabbit