docs: correct DATAFUSION-PERIMETER — two of its conclusions were wrong - #871
Conversation
Codex reviewed the first edition (a50299c, merged as #870) and found four defects. All four verified; two changed what the document concludes. Superseded text is retained in place and labelled, never silently overwritten. 1. P1 -- the corollary "Lance storage and a Pi-sized binary are mutually exclusive" was WRONG. It equated a capability with a crate. Measured in ISOLATED scratch crates (a cargo tree -p inside the workspace resolves under feature unification and is not comparable): lance = 7.0.0 -> 571 crates, 32 datafusion lance-table = 7.0.0 -> 280 crates, 1 datafusion The one is datafusion-common, non-optional, via lance-file; lance-core's datafusion deps are behind an optional feature and lance-table's manifest names DataFusion nowhere. And lance-table is where the versioning lives -- Manifest, Transaction, CommitHandler. So the trade is not "versioned storage vs memory-only" (the first edition's top open question); it is whether the Dataset conveniences this repo actually uses can be re-created on lance-table. New section 2a. 2. P1 -- ScanStrategy was never on the datafusion_planner public surface. The 6 files were crate::ir::logical_op::ScanStrategy in the planner, a different type in a different crate; the DataFusion one is used in 2 files, both INSIDE the module. Corrected membership: DataFusionPlanner, GraphPhysicalPlanner, vector_ops -- four resolved imports, not a name grep. The "three names" headline survives; its membership does not, and the proposal to move ScanStrategy into contract is withdrawn as a change whose reason did not exist. 3. P2 -- -e normal excludes build edges. Added a column: contract goes 1 -> 16 (serde_yaml, serde, glob + transitives). "Zero deps" stays true of the linked artifact and was never true of the build. 4. P2 -- the printed recipe did not reproduce its own numbers. cargo tree marks repeat subtrees with (*), so foo and "foo (*)" survive sort -u; the printed pipeline yields 828 for lance-graph, not 598. Codex inferred the counts were inflated -- they were not, cross-checked two ways. The counts were right and the recipe was wrong, which is the worse of the two: a wrong number gets challenged, an unreproducible recipe gets trusted. Also finishes section 10 Q1: the lance call-site census is done and short -- open + append + list-versions, one public signature leaking &lance::Dataset (lance_vector_search.rs:222, an API break not a refactor), and one genuinely query-side use. Gated on an unmeasured round-trip: whether hand-rolled open/append over lance-table reads lance-written datasets byte-identically. Section 9 gains a round two, because all four defects have one shape: a proxy that is cheap to compute gets published as the quantity it resembles. A grep resembles a census, a facade crate resembles a capability, an in-workspace tree resembles a closure. Section 9 already named the same-identifier-grep failure -- and section 5 was written with one four sections later, so the defence has to be mechanical, not vigilance. Board hygiene (EPIPHANIES E-A-CHEAP-PROXY-GETS-PUBLISHED-AS-THE-QUANTITY-IT- RESEMBLES-1) follows in a second commit: it is a large append-only file and re-transmitting it whole through this API risks corrupting it, so it waits for git rather than being rewritten wholesale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3RyLEbuNSHxxB3NTTrGki
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_d11659d0-4750-4b3a-9013-30d0651c02ff) |
|
@coderabbitai review Requesting the review while this is still a draft, deliberately. This PR corrects a document that shipped with four defects and merged (#870) before its review could reach it — so waiting for ready-for-review would repeat the exact failure the PR documents. Worth aiming at specifically: every number here is a re-measurement of a number the first edition got wrong, so please treat the measurement recipes as the review surface, not just the prose. In particular §2's @codex please also re-check the four findings you raised on #870 — I believe all four are addressed, but two of them ( Generated by Claude Code |
|
✅ Action performedReview finished.
|
…on finding E-A-CHEAP-PROXY-GETS-PUBLISHED-AS-THE-QUANTITY-IT-RESEMBLES-1, owed since #871 (merged via GitHub API push while the Bash permission classifier was intermittently unavailable in-session -- a 14,447-line append-only file was not safe to re-transmit whole through that path, so it waited for git). Status updated to SHIPPED: both #870 (first edition) and #871 (correction pass) are on main now. Added the process paragraph the entry was missing -- this is the THIRD occurrence this session of a merge outrunning its review (#866 review-in-flight, #868 merged while DO-NOT-MERGE, #870 merged mid- correction-pass), and a fourth near-miss (#871 itself merged before CodeRabbit's rate-limited review could run). Sharpens E-A-GUARANTEE-WITH-A-RE-EXPORTED-BYPASS-IS-NOT-A-GUARANTEE-1's rule: the reviewable window is "until the operator reads it," which can be zero, so correctness has to be front-loaded into the PR rather than caught after. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01K3RyLEbuNSHxxB3NTTrGki
Follow-up to #870, which merged while this correction pass was being written — so the uncorrected first edition was briefly on
mainand these fixes could not reach that PR.Codex reviewed the first edition and found four defects. All four verified against the tree; two changed what the document concludes. Superseded text is retained in place and labelled
⊘, per the append-only board convention — a reader who saw the first edition can find every retraction rather than a silently different document.The two that changed conclusions
P1 — "Lance storage and a Pi-sized binary are mutually exclusive" was wrong. It equated a capability with a crate. Measured in isolated scratch crates (a
cargo tree -pinside the workspace resolves under feature unification and is not comparable — that is itself one of the corrections):-e normal,build, deduped)lance = 7.0.0lance-table = 7.0.0That one is
datafusion-common, non-optional, vialance-file.lance-core's datafusion deps are behind an optionaldatafusionfeature;lance-table's own manifest names DataFusion nowhere — andlance-tableis whereManifest,Transaction, andCommitHandlerlive. So the trade is not "versioned storage vs memory-only" (the first edition's top open question); it is whether theDatasetconveniences this repo actually uses can be re-created onlance-table. New §2a.P1 —
ScanStrategywas never on thedatafusion_plannerpublic surface. The 6 files arelance-graph-planner'scrate::ir::logical_op::ScanStrategy(Cascade/Full/Index/CamPq) — a different type in a different crate. The DataFusion one (Cascade/FullScan) is used in 2 files, both inside the module. Corrected surface:DataFusionPlanner,GraphPhysicalPlanner,vector_ops— four resolved imports, not a name grep. The "three names" headline survives; its membership does not, and the follow-on proposal to moveScanStrategyintocontractis withdrawn as a change whose reason did not exist.The two measurement-hygiene fixes
P2 —
-e normalexcludes build edges. Added a column:contractgoes 1 → 16 (serde_yaml,serde,glob+ transitives; all pure Rust, all fine on a Pi). "Zero deps" stays true of the linked artifact and was never true of the build.P2 — the printed recipe did not reproduce its own numbers.
cargo treemarks repeat subtrees with(*), sofooandfoo (*)both survivesort -u; the printed pipeline yields 828 forlance-graph, not 598. Codex inferred the published counts were inflated — they were not (cross-checked two ways, both 598). The counts were right and the recipe was wrong, which is the worse of the two: a wrong number gets challenged, an unreproducible recipe gets trusted and re-run by someone who then disbelieves their own correct result.Also finished: §10 Q1
The
lancecall-site census the first edition deferred. Production surface isDataset::open+ append +dataset::Version— all of which §2a showslance-tablecarries — plus one genuine blocker:lance_vector_search.rs:222puts&lance::Datasetin a public signature, an API break rather than an internal refactor. Test/bench sites excluded and named. Explicitly still unmeasured: whether hand-rolled open/append overlance-tablereadslance-written datasets byte-identically. That round-trip gates any re-creation work — this document exists because a plausible inference was published ahead of its measurement.Why §9 grew a round two
All four defects have one shape: a proxy that is cheap to compute gets published as the quantity it resembles. A grep resembles a census; a façade crate resembles a capability; an in-workspace tree resembles a closure;
-e normalresembles build cost. Each was one command from the real measurement, and the real measurement changed the answer every time.Sharpest part: §9 already named the same-identifier-grep failure, and §5 was written with one four sections later. Documenting an anti-pattern does not immunise the document against it — the defence has to be mechanical (resolve imports, leave the workspace, count edge kinds), not vigilance.
Board hygiene — one commit still owed
EPIPHANIES.mdentryE-A-CHEAP-PROXY-GETS-PUBLISHED-AS-THE-QUANTITY-IT-RESEMBLES-1is written locally but not in this push. The permission classifier forBashhas been intermittently unavailable, so this commit went through the GitHub API instead of git;EPIPHANIES.mdis a large append-only governance file, and re-transmitting it whole through that API risks corrupting it. It follows in a second commit on this branch as soon as git is usable. Flagging rather than silently shipping an incomplete hygiene pass.Docs-only change; no code, no tests affected.
Generated by Claude Code