feat: add batch blob range reads#7864
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughChangesAdds planned blob byte-range reads across Rust and Python. Requests support row IDs, addresses, or indices, optional I/O buffer sizing and ordering, range validation, mixed blob sources, null handling, and bounded execution batches. Blob range reads
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PythonDataset
participant Dataset
participant ReadBlobRangesBuilder
participant BlobReadScheduler
PythonDataset->>Dataset: Submit row, offset, and length requests
Dataset->>ReadBlobRangesBuilder: Select rows and configure options
ReadBlobRangesBuilder->>BlobReadScheduler: Submit physical ranges in bounded batches
BlobReadScheduler-->>ReadBlobRangesBuilder: Return range payloads
ReadBlobRangesBuilder-->>PythonDataset: Return request index, row address, and bytes
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
python/python/lance/dataset.py-2279-2287 (1)
2279-2287: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe this as one API call, not one I/O operation.
Line 2280 promises a single planned I/O operation, but range planning can split work across payload batches and blob sources, issuing multiple physical reads. Say “one planned API call” instead.
Proposed fix
- Read row-specific blob-local byte ranges with one planned I/O operation. + Read row-specific blob-local byte ranges with one planned API call.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/python/lance/dataset.py` around lines 2279 - 2287, Update the docstring for the row-specific blob range-reading method to describe the operation as one planned API call rather than one planned I/O operation. Keep the remaining explanation of planning, grouping, coalescing, and scheduling unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@python/python/lance/dataset.py`:
- Around line 2279-2287: Update the docstring for the row-specific blob
range-reading method to describe the operation as one planned API call rather
than one planned I/O operation. Keep the remaining explanation of planning,
grouping, coalescing, and scheduling unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: a3b24ec9-b698-4423-93a1-b27084e3c051
📒 Files selected for processing (6)
python/python/lance/dataset.pypython/python/lance/lance/__init__.pyipython/python/tests/test_blob.pypython/src/dataset.rsrust/lance/src/dataset.rsrust/lance/src/dataset/blob.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
BubbleCal
left a comment
There was a problem hiding this comment.
I found two issues that should be addressed in the new range-read path.
BubbleCal
left a comment
There was a problem hiding this comment.
Found two correctness issues that reproduce on the current head.
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
Blob v2 exposes per-value range reads through `BlobFile`, while `Dataset.read_blobs` can batch only whole values. Python workloads that need row-specific windows therefore either schedule one call per blob or materialize complete values, losing centralized planning, bounded concurrency, and backpressure. This adds a dataset-level row-specific range contract for row IDs, indices, and addresses. Each request carries its row selector, offset, and length, so duplicates, out-of-order requests, and multiple ranges for one row remain unambiguous. Validation, physical-range planning, source grouping, coalescing, bounded scheduling, ordering, and error semantics stay in Rust; Python remains a thin collecting wrapper. ### Python user-path benchmark Real Amazon S3 validation ran in one CPython 3.12.13 process on a `c7i.4xlarge` in `us-east-2`. The workload used four 500 MiB Blob v2 values and 64 deterministic 100 KiB windows, with concurrency 16 and a 64 MiB I/O buffer. Both methods used the same requests and objects for ten repetitions, and method order rotated by repetition. The reported p50 and p95 use linear interpolation across those ten runs. The existing-path baseline used pre-opened `BlobFile` handles and a prewarmed 16-worker `ThreadPoolExecutor`, so handle lookup and executor startup were excluded in its favor. Its measured interval covered submitting 64 synchronous `BlobFile.read_range` calls through receiving every `bytes` result. The PR path measured one synchronous `Dataset.read_blob_ranges` call through receiving its collected result list. Payload validation ran outside both measured intervals. | Python user-path metric | Existing `ThreadPoolExecutor` + `BlobFile.read_range` | This PR: `Dataset.read_blob_ranges` | Benefit | | --- | ---: | ---: | ---: | | 64-range workload p50 wall-clock latency (lower is better) | 201.19 ms | 81.51 ms | 2.47x speedup | | 64-range workload p95 wall-clock latency (lower is better) | 377.65 ms | 103.74 ms | 3.64x speedup | | Logical throughput p50 (higher is better) | 31.07 MiB/s | 76.80 MiB/s | 2.47x throughput | ### Storage I/O diagnostic A separate Rust scheduler diagnostic at the same implementation commit measured the physical I/O for the same logical workload. It is used only to validate range efficiency, not as a Python latency baseline. | Storage path | Logical bytes | Physical bytes p50 | Physical requests p50 | Read amplification p50 | | --- | ---: | ---: | ---: | ---: | | `read_blob_ranges` | 6,553,600 | 6,556,844 | 72 | 1.0005x | | Whole-value `read_blobs` control | 6,553,600 | 2,097,155,244 | 136 | 320.0005x | The range API therefore reduced aggregate read amplification by 319.84x relative to the only existing dataset-level batch API. Direct S3 is intentionally omitted from the benefit table because it is a storage diagnostic control, not a Python user path. No client, OS, or S3 cache flush was performed; first-pass labels are observational and Amazon S3 server-side cache state is uncontrolled. Both measurements used the current PR head, `9c95e0324`. (cherry picked from commit aea6ded)
## Why Blob selection APIs currently expose an implementation detail of the physical read planner: null descriptors are omitted because they schedule no payload I/O. This makes result cardinality depend on descriptor contents, prevents callers from distinguishing null values from omitted rows, and leaves `read_blobs`, `read_blob_ranges`, and `take_blobs` with inconsistent contracts. This is a follow-up to #7864 and resolves the remaining null behavior described in #7899. ## Contract All blob selection APIs now return one logical result per selector or range request. Null blobs are represented explicitly, while valid empty blobs and empty ranges remain non-null empty values. The physical planner still avoids payload I/O for nulls; the logical result layer restores their selection positions. ## Breaking change This intentionally changes public return types and observable result cardinality: - Rust `take_blobs*` APIs return `Vec<Option<BlobFile>>`, and `ReadBlob::data` / `ReadBlobRange::data` are `Option<Bytes>`. - Python blob APIs return `Optional` values for null blobs. - Java `takeBlobs*` lists retain every requested position and may contain null elements. Callers that relied on null selections being omitted must now handle explicit null results. BREAKING CHANGE: Blob selection APIs now preserve every selector or request and represent null blob values explicitly instead of omitting them.
Blob v2 exposes per-value range reads through
BlobFile, whileDataset.read_blobscan batch only whole values. Python workloads that need row-specific windows therefore either schedule one call per blob or materialize complete values, losing centralized planning, bounded concurrency, and backpressure.This adds a dataset-level row-specific range contract for row IDs, indices, and addresses. Each request carries its row selector, offset, and length, so duplicates, out-of-order requests, and multiple ranges for one row remain unambiguous. Validation, physical-range planning, source grouping, coalescing, bounded scheduling, ordering, and error semantics stay in Rust; Python remains a thin collecting wrapper.
Python user-path benchmark
Real Amazon S3 validation ran in one CPython 3.12.13 process on a
c7i.4xlargeinus-east-2. The workload used four 500 MiB Blob v2 values and 64 deterministic 100 KiB windows, with concurrency 16 and a 64 MiB I/O buffer. Both methods used the same requests and objects for ten repetitions, and method order rotated by repetition. The reported p50 and p95 use linear interpolation across those ten runs.The existing-path baseline used pre-opened
BlobFilehandles and a prewarmed 16-workerThreadPoolExecutor, so handle lookup and executor startup were excluded in its favor. Its measured interval covered submitting 64 synchronousBlobFile.read_rangecalls through receiving everybytesresult. The PR path measured one synchronousDataset.read_blob_rangescall through receiving its collected result list. Payload validation ran outside both measured intervals.ThreadPoolExecutor+BlobFile.read_rangeDataset.read_blob_rangesStorage I/O diagnostic
A separate Rust scheduler diagnostic at the same implementation commit measured the physical I/O for the same logical workload. It is used only to validate range efficiency, not as a Python latency baseline.
read_blob_rangesread_blobscontrolThe range API therefore reduced aggregate read amplification by 319.84x relative to the only existing dataset-level batch API. Direct S3 is intentionally omitted from the benefit table because it is a storage diagnostic control, not a Python user path.
No client, OS, or S3 cache flush was performed; first-pass labels are observational and Amazon S3 server-side cache state is uncontrolled. Both measurements used the current PR head,
9c95e0324.