feat: support atomic conditional overwrite writes - #62
Draft
FANNG1 wants to merge 2 commits into
Draft
Conversation
Adds mode="overwrite_where" to daft_lance.write_lance: one Lance commit deletes the rows matching a SQL predicate and adds the new data, so readers see either the whole replacement or none of it. The delete runs distributed, one task per fragment, reusing the DatasetOpenContext pattern the merge and compaction paths already use. When a scalar index covers the predicate, a row-address lookup prunes the fragment list first; without one, every fragment goes to the workers rather than paying for a serialized scan on the driver. Input rows are checked against the predicate by default: overwrite_where appends the input verbatim, so a row outside the predicate would survive the next run of the same write instead of being replaced. validate_predicate=False opts out. The new mode writes exactly like an append and is normalized to it in the constructor, so the existing schema, blob-column and storage-version checks cover it -- adding a fourth mode string to those checks would have skipped them silently. Known gap, documented in write_lance and the README: Lance does not treat a concurrent append as conflicting with the Update this commits, so rows another writer adds during the overwrite survive it. test_concurrent_append_survives _the_overwrite pins that behavior. Co-authored-by: fanng <“fanng@apache.org”> Claude-Session: https://claude.ai/code/session_017b8TVnEmyN8wbXFTHJwYip
Review of the previous commit found two ways the input-row check could hurt
rather than help, both confirmed against real datasets.
The check evaluates the predicate with Daft while the delete runs with
Lance/DataFusion, and the two engines disagree on a decimal literal compared
against a narrow float column: Daft widens 0.1f32 to 0.10000000149 and calls
"score > 0.1" true, Lance narrows the literal and calls it false. A row like
that passed validation and was then never deleted, so re-running the same
write piled up duplicates -- exactly what the check exists to prevent. A
predicate reading a float32/float16 column is now refused up front.
The driver-side check also only parsed the Daft expression, never evaluated
it, so a Lance-valid predicate Daft types differently ("t >= TIMESTAMP '...'"
against a naive timestamp column, the shape the docstring advertises) failed
mid-write with a raw DaftTypeError and no pointer to validate_predicate=False.
It is now evaluated against a zero-row input typed like the table.
Also: from_pylist produces a single partition, so the per-fragment delete ran
as one task on a distributed runner. It is now repartitioned by fragment id,
except on the native runner where repartition is a no-op that only warns. The
dead concurrency parameter is gone and the docstrings that claimed the work
spread across the cluster now match what it does.
Tests: the concurrency and idempotence tests asserted so little that they
passed with the overwrite stubbed out to a no-op, and no test drove the
index-pruning path end to end. Both now assert whole row sets, and a new test
overwrites twice through an index on the predicate column -- the second run
only replaces the row the first one appended if pruning still finds that
fragment. Stubbing the overwrite to a no-op now fails 13 of 26 tests instead
of 7 of 22, and a pruning miss fails 2.
Co-authored-by: fanng <“fanng@apache.org”>
Claude-Session: https://claude.ai/code/session_017b8TVnEmyN8wbXFTHJwYip
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
mode="overwrite_where"with a requiredpredicatetodaft_lance.write_lance: one Lance commit deletes the rows matching the predicate and adds the DataFrame's rowsappendso the existing schema, Blob-column and storage-version checks cover itNo Lance-side change: this composes
LanceFragment.delete()withLanceOperation.Update.Closes #61
Semantics
start()pins the dataset version; workers write the input into new fragments exactly asappenddoes;finalizeapplies the predicate to the pinned snapshot's fragments and commits oneLanceOperation.Update(updated_fragments, removed_fragment_ids, new_fragments, fields_modified=[])against that version. Deletions write deletion files rather than rewriting data, so row addresses and the indexes built on them survive; the new fragments are unindexed and get scanned flat. Nothing matched and nothing written means no commit and no empty version.Fragment pruning is opportunistic: when
explain_planshows the filter answered by aScalarIndexQuery, a streamed row-address scan gives the exact fragment set; otherwise the delete visits every fragment, which costs the same single pass the pruning scan would have.The input check
The delete range comes from
predicatealone and the input is appended verbatim, so a row outside the predicate is never replaced by a re-run — it accumulates.validate_predicate=True(the default) rejects such rows per micropartition.That check evaluates the predicate with Daft while the delete runs with Lance, so it is only worth as much as the two engines' agreement. Two cases are refused up front, before any data is written, both confirmed against lance 8.0.0:
0.1f32to0.10000000149and callsscore > 0.1true; Lance narrows the literal and calls it false. A row like that passed the check and was then never deleted, so re-running the write piled up duplicates.TIMESTAMPliteral against a naive timestamp column. Daft parses it into a UTC-aware literal; previously only parsing was checked, so the write failed mid-flight with a rawDaftTypeError.Both name
validate_predicate=Falsein the error. The refusal is scoped to the columns the predicate actually reads, so a table that merely has a float32 column elsewhere still gets the check.Concurrency caveat (documented, not detected)
Lance does not treat a concurrent
Appendas conflicting with thisUpdate. A commit against a stale read version succeeds — verified withmax_retries=0— and rows another writer added during the overwrite survive it even when they match the predicate. Thewrite_lancedocstring and the README say so, andtest_concurrent_append_survives_the_overwritepins the behavior so a future Lance change surfaces here rather than silently.Test plan
ruff format --checkandruff checkcleandaft_lance/and the new test fileDatasetOpenContext+daft.clsshape that compaction and column merging already run on Ray, and the fragment frame is repartitioned by fragment id there (on the native runnerrepartitionis a no-op that only warns), but that path is untested here.26 tests in
tests/io/lancedb/test_overwrite_where.pycover: multi-fragment replacement in exactly one version, idempotent re-runs including rows the previous run appended, empty input, a predicate matching nothing, a fully matched fragment being removed, input rows outside the predicate with and without the opt-out, NULL rows counting as not matching, the argument-validation matrix, storage-version and schema parity withappend, invalid Lance filters, both dialect refusals, index pruning as a unit and end to end through two overwrites, scalar plus IVF_PQ index correctness after an overwrite, and a namespace-addressed table.Mutation-checked rather than assumed: stubbing the overwrite out to a no-op fails 13 of the 26, and making pruning report no fragments fails 2.
Review history
The first commit is the feature; the second is the fixes from an independent review of it — the two dialect refusals above, repartitioning the delete job (it had been running as a single task), and two tests that had asserted so little they passed with the overwrite stubbed out.