fix(planner): carry projections and early_limit into the physical plan

[hyperpolymath]

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.

[gitar-bot]

Note

Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime.
Learn more

Code Review ✅ Approved

Carries projections and early_limit into the physical plan to fix silent pushdown drops during the logical-to-physical translation. No issues found.

Auto-approved and auto-merge armed: No blocking issues found.
Please see Auto-approve Docs for details on setting custom approval criteria. — merges when pipeline and required approvals pass.

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact

gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[gitar-bot]

⚠️ Gitar auto-approved this PR but could not enable auto-merge: auto-merge is disabled for this repository — enable "Allow auto-merge" in the repository settings.

[gitar-bot]

Gitar has auto-approved this PR and enabled auto-merge (configure)