feat(checkpoint): support checkpoint on lance, stage Lance DataSink write results#7035
feat(checkpoint): support checkpoint on lance, stage Lance DataSink write results#7035everySympathy wants to merge 3 commits into
Conversation
7c24a7a to
3ae9c8b
Compare
Greptile SummaryThis PR adds the Daft core plumbing needed for checkpoint-aware Python
Confidence Score: 4/5The Rust checkpoint plumbing and Lance variant additions are clean and well-tested; the two findings are non-blocking style and efficiency observations. The checkpoint data-path changes in The Important Files Changed
Sequence DiagramsequenceDiagram
participant User as Python caller
participant DF as DataFrame.write_lance
participant Sink as LanceDataSink
participant Pipeline as Rust Pipeline
participant CkptStore as CheckpointStore
User->>DF: "write_lance(uri, mode=append, checkpoint=commit)"
DF->>Sink: "LanceDataSink(uri, ..., checkpoint=commit)"
DF->>Sink: checkpoint_commit_exists()?
alt idempotence key already committed in Lance
Sink-->>DF: True
DF->>Sink: finalize([])
Note over Sink: mark_committed() only
Sink-->>DF: MicroPartition
DF-->>User: DataFrame (fast path)
else normal path
Sink-->>DF: False
DF->>Pipeline: write_sink(sink)
Pipeline->>Sink: checkpoint_file_format()
Sink-->>Pipeline: Lance
Pipeline->>Pipeline: BlockingSinkNode.with_checkpoint(store, id_map, Lance)
loop per input batch
Pipeline->>Sink: write(micropartitions)
Sink-->>Pipeline: write_results
Pipeline->>CkptStore: stage_files(write_results, Lance)
end
Pipeline->>CkptStore: checkpoint(id)
Pipeline->>Sink: finalize(write_results)
Note over Sink: Lance table commit + mark_committed()
Sink-->>Pipeline: MicroPartition
Pipeline-->>User: DataFrame
end
Reviews (1): Last reviewed commit: "Merge branch 'main' into daft-lance-chec..." | Re-trigger Greptile |
| let root_is_write_sink = { | ||
| let mut is_write_sink = | ||
| root_is_write_sink || matches!(physical_plan, LocalPhysicalPlan::LanceWrite(_)); | ||
| if let LocalPhysicalPlan::DataSink(daft_local_plan::DataSink { data_sink_info, .. }) = | ||
| physical_plan | ||
| { | ||
| // Generic Python DataSinks are not native write sinks above. If the | ||
| // sink declares a checkpoint file format, it will use | ||
| // BlockingSinkNode::with_checkpoint below, so do not add a | ||
| // CheckpointTerminusNode around it here. | ||
| is_write_sink = | ||
| is_write_sink || datasink_checkpoint_file_format(data_sink_info)?.is_some(); | ||
| } | ||
| is_write_sink | ||
| }; |
There was a problem hiding this comment.
Double Python call to
checkpoint_file_format()
datasink_checkpoint_file_format is invoked twice for any checkpoint-enabled DataSink plan: once here inside translate_physical_plan_to_pipeline to compute is_write_sink, and a second time inside physical_plan_to_pipeline's DataSink arm to call node.with_checkpoint(...). Both calls acquire the GIL and round-trip into Python. The result is pure: it will always return the same value for the same sink object. The is_write_sink result could be threaded down into physical_plan_to_pipeline (or cached on DataSinkInfo) so the Python method is only called once per pipeline build.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def checkpoint_file_format(self) -> str | None: | ||
| """Optional checkpoint metadata format for DataSink write results. | ||
|
|
||
| Sinks that return a format opt in to having their ``write_results`` | ||
| staged in the checkpoint store. The default keeps existing DataSink | ||
| behavior unchanged. | ||
| """ | ||
| return None |
There was a problem hiding this comment.
Valid format strings not documented
The docstring says the return value is "a format" but does not enumerate what strings Daft accepts. Right now the only accepted value is "Lance" (or "lance") — any other non-None return causes a ValueError deep inside the Rust pipeline builder with a message like unsupported DataSink checkpoint file format: "Iceberg". Third-party sink authors have no way to know valid values from reading this method's contract. Document the accepted strings explicitly (e.g., Returns: "Lance" or None).
|
@rohitkulshreshtha Hi Rohit, could you help to review this PR? Thanks! |
Changes Made
This PR adds the Daft core plumbing needed for checkpoint-aware Python
DataSinkwriters. Lance is the first consumer.The design keeps Daft core format-agnostic. Daft core does not need to understand Lance fragments. Instead, a Python
DataSinkcan opt in through a small hook and ask Daft core to stage its per-inputwrite_resultsin the checkpoint store. The sink package can later read those stagedwrite_resultsand interpret them in its own format-specific final commit logic.This PR adds:
CheckpointFileFormat.Lance.FileFormat::Lancein checkpoint metadata format tags.DataSink.checkpoint_file_format()implementation that returnsNone.BlockingSinkNode::with_checkpointwiring for opt-in DataSinks.DataFrame.write_lance(..., checkpoint=...)API plumbing.finalize([])is still called so the sink can mark pending checkpoint ids committed.The Rust/Python boundary is intentionally narrow:
write_resultsin the checkpoint store.Lance.daft_lanceowns Lance-specific recovery and final commit logic.Why
Checkpointed sink recovery needs source keys and sink output metadata to share the same checkpoint boundary.
For Lance, workers write fragments before the final table commit. The final table commit happens later in driver-side
finalize(). If a task fails and the job retries, Daft source checkpointing may skip source keys that were already processed. In that case, the retry run may not reproduce thewrite_resultsfor those previously processed inputs.Daft core therefore needs a generic way to persist Python DataSink
write_resultsbefore sealing the checkpoint id.Testing
Tests added in this PR cover:
CheckpointFileFormat.Lance.DataSink.checkpoint_file_format()behavior.write_lance(..., checkpoint=...)API plumbing.This PR is paired with daft-engine/daft-lance#16, which adds Lance-specific tests for:
write_resultsrecovery;mark_committed()fails;Manual validation:
read_parquet -> write_lanceon a Ray cluster.Related Issues
#6967
Related PR
daft-engine/daft-lance#16