You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix(planner): carry projections and early_limit into the physical plan (#207)
fix(planner): carry projections and early_limit into the physical plan
`PlanNode` has four fields; `PlanStep` had six, and neither
`projections` nor
`early_limit` was among them. The optimizer computed both, then dropped
them at
the logical -> physical boundary, so the executor never saw either
pushdown.
That is worse than merely lossy, and this is the part worth flagging.
`CostModel::estimate` already *discounts* a node carrying an early limit
(`cost.rs:345-350`):
if let Some(limit) = node.early_limit {
let limit_factor = (limit as f64 / 1000.0).min(1.0);
selectivity *= limit_factor;
time_ms *= 0.5 + 0.5 * limit_factor; // At least 50% of base cost
}
So a plan was priced as cheap as half its base cost *because of* a limit
pushdown that the physical plan had no way to perform. The planner was
rewarding an optimisation it then discarded. Nothing failed loudly —
plans
simply cost less on paper than they cost to run, which is the kind of
defect
that shows up as unexplained latency rather than as an error.
Measured before the change: `early_limit` had exactly one non-test read
in the
entire tree — the cost adjustment above. `projections` had **none**;
nothing
read it after `vcl_bridge.rs` built it.
Both fields are now on `PlanStep` and populated from the node. They
carry
`#[serde(default)]`, so a `PhysicalPlan` serialised before this change
still
deserialises (absent -> `vec![]` / `None`, matching the old behaviour
rather
than inventing a limit).
This surfaced while discharging `optimize_is_permutation` (#206). The
Coq model
abstracts a node as `(modality, list condition)` — precisely the
projection that
omits the two fields being lost. The abstraction was not wrong, but it
was drawn
around the bug, which is why proving the permutation property could not
have
caught it.
Test: `test_pushdown_survives_optimization` asserts both fields survive,
and
matches steps by modality rather than index — asserting on `steps[0]`
would test
the sort order, not the pushdown. Verified as a real guard, not a
vacuous one:
with the population reverted it fails with `projection pushdown was
dropped at
the physical boundary`; with the fix it passes.
Verified: `cargo test -p verisim-planner` 118 passed / 0 failed; `cargo
clippy
-p verisim-planner --all-targets` clean; `cargo check -p verisim-api
--all-targets` clean (it maps `PlanStep` into its own GraphQL type);
`reuse
lint` compliant.
Not addressed here, and deliberately so: `pushed_predicates` remains
`Vec<String>` built by `format!("{:?}", c)`, so conditions still cross
the
boundary as Rust `Debug` output rather than as `ConditionKind`. Retyping
it is a
larger change with its own serialisation surface, and it is not needed
to stop
the cost model lying.
0 commit comments