feat(core): PR 4 of 8 — IncrementalAuto capability-aware dispatcher#4564
Closed
AceHack wants to merge 2 commits into
Closed
feat(core): PR 4 of 8 — IncrementalAuto capability-aware dispatcher#4564AceHack wants to merge 2 commits into
AceHack wants to merge 2 commits into
Conversation
…aces onto Op<'T> base class
PR 1 of an 8-PR campaign that wires the algebra-capability system from
declarative-but-unenforced markers into a load-bearing, uniformly-detected
property surface on every operator (internal + plugin).
## What changes
`Op` base class (Circuit.fs) gains four abstract properties — `IsLinear`,
`IsBilinear`, `IsSink`, `IsStatefulStrict` — each defaulting to `false`.
Concrete operators override only the capabilities they actually have.
Until this change, the algebra tags lived ONLY as plugin marker
interfaces in PluginApi.fs and were ignored by `PluginOperatorAdapter`
(which detected `IStrictOperator`/`IAsyncOperator`/`INestedFixpointParticipant`
but not the algebra markers). That asymmetry meant:
- Internal operators (MapZSetOp, JoinZSetOp, etc.) had no capability
surface at all — algebra was implicit-by-code-shape.
- Plugin operators declared capabilities via marker interfaces but
`PluginOperatorAdapter` discarded the declarations.
- Consumers (Incremental.IncrementalJoin, future Fusion/IncrementalAuto)
had no uniform way to ask "is this operator linear?" without
custom type tests per call site.
## Non-generic marker pattern
F# generic-interface tests require exact type-parameter match —
`(box plugin) :? IBilinearOperator<obj, obj, 'TOut>` against a concrete
`IBilinearOperator<int, string, decimal>` returns false. The fix is the
BCL `IEnumerable` / `IEnumerable<T>` pattern: a non-generic marker
interface (`ILinearMarker`, `IBilinearMarker`, `ISinkMarker`,
`IStatefulStrictMarker`) for runtime `:?` tests, and the typed interface
inheriting the marker. Plugin authors continue implementing the typed
interface; the marker is satisfied automatically via interface
inheritance.
`PluginOperatorAdapter` now caches one `:?` check per marker at
construction (zero per-tick cost) and surfaces the results through
the new `Op` overrides.
## Internal-operator overrides
| Operator | Capability | Reasoning |
|---|---|---|
| MapZSetOp, FilterZSetOp, FlatMapZSetOp, NegZSetOp | IsLinear=true | Z-set algebra: distributes over addition, op(0)=0 |
| IndexWithOp | IsLinear=true | Indexing distributes over per-key value-group sum |
| JoinZSetOp, CartesianZSetOp, IndexedJoinOp | IsBilinear=true | Weights multiply; per-arg linear; op(0,b)=op(a,0)=0 |
| DelayOp, IntegrateOp, DifferentiateOp | IsLinear=true | Time-shift / running-sum / difference commute with group |
| FilterMapOp, FilterMapOptionalOp | IsLinear=true | Composition of linear ops |
| PlusZSetOp, MinusZSetOp | (default false) | Additive but NOT unary-linear: Plus(0,b)=b≠0 |
| DistinctZSetOp, DistinctIncrementalOp | (default false) | Clamps weights — breaks linearity |
| GroupBySumOp | (default false) | Output keys depend on summed weights, breaks linearity |
| ConstantOp | (default false) | Affine; const_c(0)=c≠0 unless c=0 |
## Tests
21 new tests in `tests/Tests.FSharp/Plugin/Capabilities.Tests.fs`:
- 15 internal-operator capability tests (one per named op)
- 5 plugin-marker-detection tests via PluginOperatorAdapter
- 1 negative test: plain IOperator plugin reports all caps false
All 31 plugin tests pass (10 pre-existing + 21 new); 480 / 481 broader
operator/algebra/circuit tests pass (1 SKIP is pre-existing). Build
clean: 0 warnings, 0 errors on full solution Release build.
## Foundation for PRs 2-8
This is the load-bearing dependency for:
- PR 2: Circuit.Build() consults IsSink for terminal-placement
enforcement (the docstring promise that's currently vapor).
- PR 4: IncrementalAuto dispatcher reads IsLinear/IsBilinear to
pick Q^Δ=Q vs three-term-bilinear vs D∘Q∘I fallback.
- PR 5: FusionEngine composes capability tags through DAG rewrite.
- PRs 6-8: push/morsel/codegen architectures all need uniform
capability surfacing to dispatch correctly.
No public-API breakage: the marker interfaces still work the same
way for plugin authors; the new Op-base-class properties are
purely additive.
Adds `Circuit.IncrementalAuto<'K>(q, input)`, a dispatcher that picks
the right incrementalization based on the algebra capability tag on
`q`'s resulting operator:
- `IsLinear = true` → `Q^Δ = Q` (deltas pass through unchanged)
- `IsSink = true` → throws (sinks are terminal, can't incrementalize)
- otherwise → fall back to `D ∘ Q ∘ I` via existing
`IncrementalizeZSet`
This makes the algebra-capability system load-bearing on the
incremental-rewrite side — the DBSP paper's central claim "linear ops
incrementalize trivially; bilinear ops use the three-term form; rest
fall back to D∘Q∘I" finally has a dispatcher that mechanizes the first
and third clauses. (The bilinear case has its own richer signature in
`IncrementalJoin`; a future `IncrementalAutoJoin` dispatcher for
bilinear ops can layer on top of that.)
## How dispatch works
The dispatcher probes `q` by applying to the input directly, then
inspects `Op.IsLinear` / `Op.IsSink` (from PR 1, #4558). The probe
side-effect registers the operator in the circuit; for the
linear-passthrough path this IS the correct wiring. For the
non-linear fallback path, the probed op is orphan (no consumers) — a
small per-tick cost; pruning unreachable operators at `Circuit.Build()`
is a future improvement.
## Tests (5 new, all pass)
- `IncrementalAuto with linear Map produces same delta stream as
direct Q` — operational correctness check over 4 ticks (insert,
insert, retract, empty)
- `IncrementalAuto with non-linear Distinct falls back to D-Q-I` —
output matches `IncrementalizeZSet` over a 6-tick scenario with
duplicates and retractions
- `IncrementalAuto throws when the operator is a sink` — error
message contains "IncrementalAuto", "sink", and the operator name
- `IncrementalAuto with linear op adds exactly one operator` —
structural check that the fast path was taken (just the Map; no I
or D)
- `IncrementalAuto with non-linear op adds four operators` —
structural check that fallback registered probe + I + new Q + D
(probe is the documented orphan)
No regressions in broader Circuit tests. Build clean.
## Dependency
PR 4 depends on PR 1 (#4558) for `Op.IsLinear` and `Op.IsSink`.
Stacked on `feat/op-capability-tags-2026-05-21`; targets `main`.
## Foundation for later PRs
PR 5's FusionEngine will use the same `IsLinear`/`IsBilinear` reads
to compose capability tags through fusion. PR 8's standing-query
codegen can generate the `IncrementalAuto` decision tree at compile
time rather than at runtime probe, eliminating the orphan-operator
cost entirely.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30fb3986a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+132
to
+143
| let resultOp = probedOutput.Op | ||
| if resultOp.IsSink then | ||
| invalidOp | ||
| (sprintf | ||
| "IncrementalAuto: cannot incrementalize a sink operator. \ | ||
| '%s' (id=%d) declared IsSink=true; sinks are terminal \ | ||
| and excluded from relational composition. Use a \ | ||
| non-sink operator, or consume the sink's output \ | ||
| directly without incrementalization." | ||
| resultOp.Name resultOp.Id) | ||
| elif resultOp.IsLinear then | ||
| // Q^Δ = Q. For linear q, q(delta) IS the delta of q(full). |
There was a problem hiding this comment.
IncrementalAuto decides the strategy from probedOutput.Op.IsLinear, which only describes the final operator in q, not the whole q graph. This misclassifies composite queries such as q = map ∘ distinct: the terminal map is linear, so the dispatcher returns q(delta) even though distinct upstream is non-linear and requires D∘Q∘I. In that case the second duplicate delta is emitted incorrectly instead of being suppressed, so incremental results diverge from the true query delta.
Useful? React with 👍 / 👎.
| /// trivially when `initial = 0` for the group. Callers passing a | ||
| /// non-zero initial are responsible for the resulting affine | ||
| /// offset — DBSP usage always passes the group zero. | ||
| override _.IsLinear = true |
There was a problem hiding this comment.
DelayOp is marked IsLinear = true unconditionally, but Delay(s, initial) is affine when initial is non-zero (delay(0) emits initial on the first tick). With IncrementalAuto, any query containing Delay with a non-empty initial Z-set can be routed down the linear fast path (Q^Δ = Q) and produce wrong deltas on the first tick. The capability should only be linear when the initial value is the group zero.
Useful? React with 👍 / 👎.
Member
Author
Loop maintenance: rebase ready on a new branchPR #4558 (the capability-tags foundation this PR was stacked on) merged to main, leaving this PR in I rebased PR 4's content cleanly onto current The rebased branch is pushed: To update this PR, the cleanest options are:
Both work; (1) preserves this PR's number + discussion thread, (2) is fully non-destructive. I'm pausing here per the loop instructions on irreversible actions. The rebased branch is parked and ready. |
There was a problem hiding this comment.
Pull request overview
Adds a capability-aware incrementalization dispatcher (Circuit.IncrementalAuto) and wires algebra capability tags (IsLinear/IsSink/IsBilinear/IsStatefulStrict) through core operators and plugin adapters, with new tests covering both internal ops and plugin marker detection.
Changes:
- Introduces
Circuit.IncrementalAuto<'K>to dispatch between trivial linear incrementalization, sink rejection, andD ∘ Q ∘ Ifallback. - Promotes algebra capability tags to
Opand implements marker-based capability detection for plugin operators. - Adds test coverage for capability tagging/detection and for
IncrementalAutobehavior and structure.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Tests.FSharp/Tests.FSharp.fsproj | Registers new test files in compile order. |
| tests/Tests.FSharp/Plugin/Capabilities.Tests.fs | Tests internal-op tags + plugin marker detection. |
| tests/Tests.FSharp/Circuit/IncrementalAuto.Tests.fs | End-to-end + structural tests for IncrementalAuto. |
| src/Core/Primitive.fs | Marks Delay/Integrate/Differentiate as linear; adds rationale comments. |
| src/Core/PluginApi.fs | Adds non-generic capability markers and adapter detection; updates docs. |
| src/Core/Operators.fs | Tags core Z-set ops as linear/bilinear with explanatory comments. |
| src/Core/Incremental.fs | Implements IncrementalAuto dispatcher. |
| src/Core/Fusion.fs | Tags fused filter-map operators as linear. |
| src/Core/Circuit.fs | Adds capability-tag properties to Op base class with docstrings. |
Comments suppressed due to low confidence (2)
src/Core/Circuit.fs:79
- P1:
IsSinkdocstring claimsCircuit.Build()rejects operators reading from a sink (terminal-placement enforcement), butCircuit.Build()currently only performs topological scheduling and cycle checks and does not inspectIsSink. Either adjust this to a future-tense note (if coming in a later stacked PR) or implement the enforcement in the same change set to keep docs accurate.
/// Algebra capability: operator is a *sink* — terminal,
/// retraction-lossy, may emit a non-Z-set output. Sinks are
/// excluded from relational composition: `Circuit.Build()` rejects
/// any operator that reads from a sink's output stream (terminal-
/// placement enforcement). Bayesian aggregates and external-system
/// sinks are canonical examples.
src/Core/PluginApi.fs:155
- P1:
ISinkOperatordocs state sink terminal placement is enforced via aCircuit.Build()validation pass, butCircuit.Build()currently does not checkIsSink/ISinkMarkerat all. Please avoid claiming enforcement that isn't implemented yet (or land the enforcement alongside this doc change).
/// Algebra capability: the operator is a *sink* — terminal,
/// non-Z-set-emitting, potentially retraction-lossy. Sink
/// operators are consciously exempt from relational
/// composition laws and the scheduler enforces terminal
/// placement (a sink may not feed another operator inside a
/// relational path) via the `Circuit.Build()` validation pass.
/// Bayesian aggregates are the canonical example.
type ISinkOperator<'TIn, 'TOut> =
Comment on lines
+17
to
+21
| /// Linear: `z⁻¹` is a time-shift; it distributes over addition | ||
| /// trivially when `initial = 0` for the group. Callers passing a | ||
| /// non-zero initial are responsible for the resulting affine | ||
| /// offset — DBSP usage always passes the group zero. | ||
| override _.IsLinear = true |
Comment on lines
+69
to
+70
| /// `IncrementalAuto` uses this to emit the three-term incremental | ||
| /// form `Δa ⋈ Δb + z⁻¹(I(a)) ⋈ Δb + Δa ⋈ z⁻¹(I(b))`. |
Comment on lines
129
to
134
| /// Algebra capability: the operator is *linear* — `op(a + b) = | ||
| /// op(a) + op(b)` and `op(0) = 0`. Retraction-native: a | ||
| /// negative weight un-accumulates correctly. Declared at the | ||
| /// type level so the scheduler can run `LinearLaw` at | ||
| /// `Circuit.Build()`. | ||
| /// `Circuit.Build()` (test-time, via `LawRunner.checkLinear`). | ||
| type ILinearOperator<'TIn, 'TOut> = |
Comment on lines
+123
to
+124
| /// and 0 ⋈ b = a ⋈ 0 = 0. IncrementalAuto rewrites this to the | ||
| /// three-term form `Δa ⋈ Δb + z⁻¹(I(a)) ⋈ Δb + Δa ⋈ z⁻¹(I(b))`. |
Comment on lines
+29
to
+40
| let private feedAndStep | ||
| (c: Circuit) | ||
| (input: ZSetInputHandle<int>) | ||
| (handle: OutputHandle<ZSet<int>>) | ||
| (deltas: ZSet<int> list) | ||
| : ZSet<int> list = | ||
| [ for delta in deltas -> | ||
| input.Send delta | ||
| c.Step() | ||
| handle.Current ] | ||
|
|
||
|
|
|
|
||
|
|
||
| [<Fact>] | ||
| let ``IncrementalAuto with non-linear op adds three operators (Integrate + Q-orphan + new Q + Differentiate)`` () = |
Member
Author
|
Closing in favor of replacement PR re-landed on current main via cherry-pick. Same content (commit |
3 tasks
auto-merge was automatically disabled
May 21, 2026 17:45
Pull request was closed
AceHack
added a commit
that referenced
this pull request
May 21, 2026
…push-based + morsel + codegen capstone (#4568) * backlog(B-0692+B-0693+B-0694): Otto-VSCode 8-PR campaign PRs 6-7-8 — push-based hot-path (IPushOperator + segment-detection) + morsel/span execution (IMorselOperator + cache-sized chunks) + standing-query codegen (IIncrementalGenerator + F# Type Provider) capstone; Aaron-approved shadow* 'file the 3 rows for PRs 6-8'; depends_on chain to PRs 1-5 substrate (#4558/#4560/#4566 merged + #4563/#4564 pending) * fix(md-lint): MD022/MD032 blanks-around-headings/lists on B-069[234] rows — Phase N subheadings + immediate-bullets need blank lines per markdownlint-cli2 * fix(reviewer-threads): resolve 6 unresolved P1/P2 findings on B-0692/B-0693/B-0694 — (a) move B-0635 + B-0688 from hard depends_on to composes_with per Codex P2 (narrative says PR #1-#5 are the real prereqs; B-0635 wave-particle is conceptual cousin; B-0688 doesn't even exist on main yet so dangling hard-edge); (b) correct Op.fs path references to acknowledge Op<'T> lives in src/Core/Circuit.fs (Copilot P1 — file doesn't exist); (c) mark proposed-new directories in B-0694 Phase 2/3 as TO BE CREATED (Copilot P1 — paths don't exist today)
AceHack
added a commit
that referenced
this pull request
May 21, 2026
…re-land of #4564) (#4567) * feat(core): IncrementalAuto capability-aware dispatcher — PR 4 of 8 Adds `Circuit.IncrementalAuto<'K>(q, input)`, a dispatcher that picks the right incrementalization based on the algebra capability tag on `q`'s resulting operator: - `IsLinear = true` → `Q^Δ = Q` (deltas pass through unchanged) - `IsSink = true` → throws (sinks are terminal, can't incrementalize) - otherwise → fall back to `D ∘ Q ∘ I` via existing `IncrementalizeZSet` This makes the algebra-capability system load-bearing on the incremental-rewrite side — the DBSP paper's central claim "linear ops incrementalize trivially; bilinear ops use the three-term form; rest fall back to D∘Q∘I" finally has a dispatcher that mechanizes the first and third clauses. (The bilinear case has its own richer signature in `IncrementalJoin`; a future `IncrementalAutoJoin` dispatcher for bilinear ops can layer on top of that.) The dispatcher probes `q` by applying to the input directly, then inspects `Op.IsLinear` / `Op.IsSink` (from PR 1, #4558). The probe side-effect registers the operator in the circuit; for the linear-passthrough path this IS the correct wiring. For the non-linear fallback path, the probed op is orphan (no consumers) — a small per-tick cost; pruning unreachable operators at `Circuit.Build()` is a future improvement. - `IncrementalAuto with linear Map produces same delta stream as direct Q` — operational correctness check over 4 ticks (insert, insert, retract, empty) - `IncrementalAuto with non-linear Distinct falls back to D-Q-I` — output matches `IncrementalizeZSet` over a 6-tick scenario with duplicates and retractions - `IncrementalAuto throws when the operator is a sink` — error message contains "IncrementalAuto", "sink", and the operator name - `IncrementalAuto with linear op adds exactly one operator` — structural check that the fast path was taken (just the Map; no I or D) - `IncrementalAuto with non-linear op adds four operators` — structural check that fallback registered probe + I + new Q + D (probe is the documented orphan) No regressions in broader Circuit tests. Build clean. PR 4 depends on PR 1 (#4558) for `Op.IsLinear` and `Op.IsSink`. Stacked on `feat/op-capability-tags-2026-05-21`; targets `main`. PR 5's FusionEngine will use the same `IsLinear`/`IsBilinear` reads to compose capability tags through fusion. PR 8's standing-query codegen can generate the `IncrementalAuto` decision tree at compile time rather than at runtime probe, eliminating the orphan-operator cost entirely. * fix(PR #4567 review threads): probe-side-effect docs + test naming + dead-code cleanup Addresses 5 reviewer findings on PR #4567: 1. **Codex P1** (Incremental.fs IncrementalAuto): probe `q.Invoke input` runs *before* dispatch decides path, so side-effecting builders (e.g. `AdvancedExtensions.Inspect`) execute unexpected work even on sink/fallback paths. Sink case throws *after* registering the probe op, leaving an orphan sink in the circuit if the caller catches + continues. **Fix**: prominent⚠️ side-effect warning section in docstring naming both implications; sink-throw error message also calls out the orphan-sink left behind. Architectural fix (non-registering probe path) is non-trivial and deferred — acknowledged in the new "future work" line. The docstring now makes the side-effect explicit so callers can avoid the failure mode (use non-side-effecting `q`; don't catch+continue on a sink-rejection). 2. **Copilot** (IncrementalAuto.Tests.fs:39): `feedAndStep` helper was dead code (left after refactoring out the test that used it). **Fix**: removed. 3. **Copilot** (IncrementalAuto.Tests.fs:176): test name said "adds three operators" but assertion + comment expected 4. **Fix**: renamed to "adds four operators (probe-orphan + Integrate + new Q + Differentiate)" matching the assertion. 4. **Copilot** (IncrementalAuto.Tests.fs:142): comment block claimed "output stream is literally the probed op (same reference)" and "zero-allocation" — but the test only asserts operator-count delta, not reference identity. **Fix**: rephrase comment to match what's actually tested (dispatch path verified via operator-count delta; reference-equality assertion would need internal `Stream.Op` accessor exposed). ## Tests 5/5 IncrementalAuto tests pass after fixes. Build clean. No behavioral changes to operator dispatch — the test rename + comment edits + dead-code removal are all annotation-shape; the docstring side-effect warnings are documentation-only. The orphan-sink + probe- side-effect *behavior* is unchanged; the *visibility* of that behavior is improved per Codex's P1. ## Architectural follow-up not in scope The cleanest fix for both Codex's P1 (probe side effects) AND the sink-rejection orphan-sink issue would be a non-registering probe path — e.g., a `Circuit.PreviewOp(q, input)` that runs `q.Invoke` inside a transaction-shaped scope that rolls back registration if the dispatcher rejects. That requires changes to the `Op` / `Circuit` registration contract (today's `Register` doesn't support rollback) and is deferred to a future PR. Worth filing as a backlog row when the architectural cost vs. payoff is clearer. * fix(PR #4567 Codex P1 #2): IncrementalAuto must check whole-chain linearity, not just terminal op Codex's second P1 finding caught a real correctness bug I missed: the dispatcher was checking `probedOutput.Op.IsLinear`, but `probedOutput.Op` is only the terminal operator in the chain produced by `q`. A query like `q(s) = Map(Distinct(s))` ends on a linear Map but the *composed* query is non-linear; the `Q^Δ = Q` rewrite produces wrong incremental results. ## Fix Recursive chain check: walk `Inputs` back from the probed terminal op to the original `input.Op`. If every op in the chain is linear AND we reach `input.Op` only through linear ops, the chain is linear. Otherwise fall back to `D ∘ Q ∘ I`. Multi-input ops (Plus, Minus, default IsLinear=false) correctly route to the fallback path via this check. ```fsharp let rec isLinearChainToInput (op: Op) (inputOp: Op) : bool = if System.Object.ReferenceEquals(op, inputOp) then true elif op.Inputs.Length = 0 then false // source op that isn't the input elif not op.IsLinear then false else op.Inputs |> Array.forall (fun dep -> isLinearChainToInput dep inputOp) ``` ## Regression test added `IncrementalAuto with terminal-linear-but-inner-non-linear chain falls back (Map ∘ Distinct)` — exercises the exact failure mode Codex flagged. Builds Map(Distinct(s)), runs IncrementalAuto, compares per-tick output to IncrementalizeZSet (D∘Q∘I reference). The test exercises duplicate-insertion + retraction scenarios where the broken dispatcher would produce wrong output (Map of every delta directly, ignoring Distinct's cumulative-state clamping). Before this fix: subject would emit `Map(delta)` directly each tick → incorrect when delta has duplicates that Distinct clamps. After this fix: subject falls back to D∘Q∘I → matches reference. ## Verification 6/6 IncrementalAuto tests pass (5 pre-existing + 1 new regression). Build clean. The chain-check helper is O(N) in chain depth, called once at dispatch time; zero per-tick cost. ## Related Composes with the earlier fix in this PR for the other 5 review threads (8230bed) — docstring side-effect warnings, test rename, dead-code removal, comment rewording. This commit addresses the substantive correctness P1 that the earlier docstring-shape fixes left unaddressed.
AceHack
added a commit
that referenced
this pull request
May 21, 2026
…re-land of #4564) (#4567) * feat(core): IncrementalAuto capability-aware dispatcher — PR 4 of 8 Adds `Circuit.IncrementalAuto<'K>(q, input)`, a dispatcher that picks the right incrementalization based on the algebra capability tag on `q`'s resulting operator: - `IsLinear = true` → `Q^Δ = Q` (deltas pass through unchanged) - `IsSink = true` → throws (sinks are terminal, can't incrementalize) - otherwise → fall back to `D ∘ Q ∘ I` via existing `IncrementalizeZSet` This makes the algebra-capability system load-bearing on the incremental-rewrite side — the DBSP paper's central claim "linear ops incrementalize trivially; bilinear ops use the three-term form; rest fall back to D∘Q∘I" finally has a dispatcher that mechanizes the first and third clauses. (The bilinear case has its own richer signature in `IncrementalJoin`; a future `IncrementalAutoJoin` dispatcher for bilinear ops can layer on top of that.) The dispatcher probes `q` by applying to the input directly, then inspects `Op.IsLinear` / `Op.IsSink` (from PR 1, #4558). The probe side-effect registers the operator in the circuit; for the linear-passthrough path this IS the correct wiring. For the non-linear fallback path, the probed op is orphan (no consumers) — a small per-tick cost; pruning unreachable operators at `Circuit.Build()` is a future improvement. - `IncrementalAuto with linear Map produces same delta stream as direct Q` — operational correctness check over 4 ticks (insert, insert, retract, empty) - `IncrementalAuto with non-linear Distinct falls back to D-Q-I` — output matches `IncrementalizeZSet` over a 6-tick scenario with duplicates and retractions - `IncrementalAuto throws when the operator is a sink` — error message contains "IncrementalAuto", "sink", and the operator name - `IncrementalAuto with linear op adds exactly one operator` — structural check that the fast path was taken (just the Map; no I or D) - `IncrementalAuto with non-linear op adds four operators` — structural check that fallback registered probe + I + new Q + D (probe is the documented orphan) No regressions in broader Circuit tests. Build clean. PR 4 depends on PR 1 (#4558) for `Op.IsLinear` and `Op.IsSink`. Stacked on `feat/op-capability-tags-2026-05-21`; targets `main`. PR 5's FusionEngine will use the same `IsLinear`/`IsBilinear` reads to compose capability tags through fusion. PR 8's standing-query codegen can generate the `IncrementalAuto` decision tree at compile time rather than at runtime probe, eliminating the orphan-operator cost entirely. * fix(PR #4567 review threads): probe-side-effect docs + test naming + dead-code cleanup Addresses 5 reviewer findings on PR #4567: 1. **Codex P1** (Incremental.fs IncrementalAuto): probe `q.Invoke input` runs *before* dispatch decides path, so side-effecting builders (e.g. `AdvancedExtensions.Inspect`) execute unexpected work even on sink/fallback paths. Sink case throws *after* registering the probe op, leaving an orphan sink in the circuit if the caller catches + continues. **Fix**: prominent⚠️ side-effect warning section in docstring naming both implications; sink-throw error message also calls out the orphan-sink left behind. Architectural fix (non-registering probe path) is non-trivial and deferred — acknowledged in the new "future work" line. The docstring now makes the side-effect explicit so callers can avoid the failure mode (use non-side-effecting `q`; don't catch+continue on a sink-rejection). 2. **Copilot** (IncrementalAuto.Tests.fs:39): `feedAndStep` helper was dead code (left after refactoring out the test that used it). **Fix**: removed. 3. **Copilot** (IncrementalAuto.Tests.fs:176): test name said "adds three operators" but assertion + comment expected 4. **Fix**: renamed to "adds four operators (probe-orphan + Integrate + new Q + Differentiate)" matching the assertion. 4. **Copilot** (IncrementalAuto.Tests.fs:142): comment block claimed "output stream is literally the probed op (same reference)" and "zero-allocation" — but the test only asserts operator-count delta, not reference identity. **Fix**: rephrase comment to match what's actually tested (dispatch path verified via operator-count delta; reference-equality assertion would need internal `Stream.Op` accessor exposed). ## Tests 5/5 IncrementalAuto tests pass after fixes. Build clean. No behavioral changes to operator dispatch — the test rename + comment edits + dead-code removal are all annotation-shape; the docstring side-effect warnings are documentation-only. The orphan-sink + probe- side-effect *behavior* is unchanged; the *visibility* of that behavior is improved per Codex's P1. ## Architectural follow-up not in scope The cleanest fix for both Codex's P1 (probe side effects) AND the sink-rejection orphan-sink issue would be a non-registering probe path — e.g., a `Circuit.PreviewOp(q, input)` that runs `q.Invoke` inside a transaction-shaped scope that rolls back registration if the dispatcher rejects. That requires changes to the `Op` / `Circuit` registration contract (today's `Register` doesn't support rollback) and is deferred to a future PR. Worth filing as a backlog row when the architectural cost vs. payoff is clearer. * fix(PR #4567 Codex P1 #2): IncrementalAuto must check whole-chain linearity, not just terminal op Codex's second P1 finding caught a real correctness bug I missed: the dispatcher was checking `probedOutput.Op.IsLinear`, but `probedOutput.Op` is only the terminal operator in the chain produced by `q`. A query like `q(s) = Map(Distinct(s))` ends on a linear Map but the *composed* query is non-linear; the `Q^Δ = Q` rewrite produces wrong incremental results. ## Fix Recursive chain check: walk `Inputs` back from the probed terminal op to the original `input.Op`. If every op in the chain is linear AND we reach `input.Op` only through linear ops, the chain is linear. Otherwise fall back to `D ∘ Q ∘ I`. Multi-input ops (Plus, Minus, default IsLinear=false) correctly route to the fallback path via this check. ```fsharp let rec isLinearChainToInput (op: Op) (inputOp: Op) : bool = if System.Object.ReferenceEquals(op, inputOp) then true elif op.Inputs.Length = 0 then false // source op that isn't the input elif not op.IsLinear then false else op.Inputs |> Array.forall (fun dep -> isLinearChainToInput dep inputOp) ``` ## Regression test added `IncrementalAuto with terminal-linear-but-inner-non-linear chain falls back (Map ∘ Distinct)` — exercises the exact failure mode Codex flagged. Builds Map(Distinct(s)), runs IncrementalAuto, compares per-tick output to IncrementalizeZSet (D∘Q∘I reference). The test exercises duplicate-insertion + retraction scenarios where the broken dispatcher would produce wrong output (Map of every delta directly, ignoring Distinct's cumulative-state clamping). Before this fix: subject would emit `Map(delta)` directly each tick → incorrect when delta has duplicates that Distinct clamps. After this fix: subject falls back to D∘Q∘I → matches reference. ## Verification 6/6 IncrementalAuto tests pass (5 pre-existing + 1 new regression). Build clean. The chain-check helper is O(N) in chain depth, called once at dispatch time; zero per-tick cost. ## Related Composes with the earlier fix in this PR for the other 5 review threads (8230bed) — docstring side-effect warnings, test rename, dead-code removal, comment rewording. This commit addresses the substantive correctness P1 that the earlier docstring-shape fixes left unaddressed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Circuit.IncrementalAuto<'K>(q, input)— a dispatcher that picks the right incrementalization based onq's resulting operator's algebra capability tag:IsLinear = trueQ^Δ = Q(deltas pass through)IsSink = trueD ∘ Q ∘ Ifallback via existingIncrementalizeZSetPR 4 of 8. Depends on PR 1 (#4558) for
Op.IsLinear/Op.IsSink. Stacked onfeat/op-capability-tags-2026-05-21.Why
Makes load-bearing the DBSP paper's central claim — "linear operators incrementalize trivially; bilinear operators use the three-term form; the rest fall back to D∘Q∘I" — by mechanizing the first and third clauses at the dispatcher level. (Bilinear case has its own richer signature in
IncrementalJoin; a futureIncrementalAutoJoincan layer on top.)Implementation note: probe + dispatch
The dispatcher probes
qby applying to the input directly, readsOp.IsLinear/Op.IsSink, then either:IncrementalizeZSet, leaving the probed op orphan in the circuit (small per-tick cost; pruning unreachable operators atCircuit.Build()is a future improvement).Tests (5 new, all pass)
IncrementalizeZSet)Foundation for later PRs
IncrementalAutodecision tree at compile time, eliminating the orphan-operator cost entirely.Test plan
dotnet buildcleandotnet test --filter IncrementalAutoTests— 5/5 pass