feat(python): plan chunk-shuffled reads with IndexedSplit - #890
Conversation
leaves12138
left a comment
There was a problem hiding this comment.
Found a user-visible correctness regression in the existing row-range read API, reproduced with a freshly built runtime from this PR. This is independent of compatibility with intermediate/unreleased split implementations: no chunk shuffle is used in the reproducer. See the inline comment for details.
| let range_base = if data_evolution { | ||
| file_meta.first_row_id.unwrap_or(0) | ||
| } else { | ||
| split_file_offset |
There was a problem hiding this comment.
[P1] Preserve ordinary row-ID selections on row-tracking append tables
The existing planner still places absolute row IDs into splits for ReadBuilder.with_row_ranges(...): split_row_ranges_for_files intersects the requested IDs with each file's first_row_id range. This branch now interprets every non-DE split as split-local physical offsets, including ordinary row-tracking scans that never call with_chunk_shuffle. The planner and reader therefore use different coordinate systems.
Reproduced with this PR's actual Python binding: create an append table with row-tracking.enabled=true (leave data evolution disabled), set both source.split.target-size and source.split.open-file-cost to 1b, and make two commits containing IDs [10, 11, 12] and [20, 21, 22]. Then read the splits from:
builder = table.new_read_builder().with_row_ranges([(3, 4)])
splits = builder.new_scan().plan().splits()
batches = list(builder.new_read().read(splits))The control runtime returns IDs [20, 21]; this PR returns no batches. Selecting [(1, 4)] similarly returns only [11, 12] instead of [11, 12, 20, 21]. The second file has first_row_id=3, but its split-local offset is zero, so its selected rows are discarded. I also verified the base-branch data-file reader matches the control reader.
Please make the existing row-ID planning path and the new raw chunk path agree on coordinates (for example, translate absolute selections into split-local ranges when constructing raw splits), and add a regression test with multiple row-tracking files whose first row IDs are nonzero. This does not require retaining an unpublished wire format, but must avoid silently changing the results of the existing user-facing API.
leaves12138
left a comment
There was a problem hiding this comment.
Re-reviewed 7596b12. The original ordinary row-ID range-read regression is fixed: the exact previous reproducer now returns the expected rows. The 89 binding read tests and 543 targeted companion PyPaimon tests also pass.
There is still a user-visible coordinate mismatch with the current companion apache/paimon#10023 (fb79b662af27a5c55f1982cf7e562b812eb9e74c), detailed inline. My additional row-tracking/non-row-tracking x Python/native planning x Python/native reading matrix passes all eight combinations before this fix, but the two mixed-backend row-tracking cases now silently drop rows. This uses both current PR heads, not an old or intermediate runtime. Please align the companion's range producers/readers before approval of the paired change.
| let range_base = if row_tracking { | ||
| segment | ||
| .input | ||
| .file | ||
| .first_row_id |
There was a problem hiding this comment.
[P1] Align the companion Python paths with the new row-tracked range coordinates
Using first_row_id here makes the Rust planner/reader agree, but the current companion #10023 still uses split-local coordinates for all non-DE append IndexedSplits. Its AppendChunkShuffleSplitGenerator._chunk_to_split builds ranges from split_offset, and RawFileSplitRead.__init__ always passes ranges through _split_local_row_ranges_by_file, including on row-tracking tables.
Reproduced against freshly built 7596b12 plus companion fb79b66: create a non-DE append table with row-tracking.enabled=true, commit [0, 1, 2] and [3, 4, 5] as two files, then use this normal public API:
builder = table.copy({
'scan.native-plan.enabled': 'true',
'read.native.enabled': 'false',
}).new_read_builder()
plan = builder.new_scan().with_chunk_shuffle(42, 3).plan()
rows = builder.new_read().to_arrow(plan.splits())Expected IDs are [0, 1, 2, 3, 4, 5]; only [0, 1, 2] are returned. Rust emits [3, 5] for the second single-file split, but Python treats that as a local range for a three-row file and discards the whole chunk.
The reverse supported path also fails: plan with both native options disabled, then feed those Python-planned splits to a reader with read.native.enabled=true. Python emits [0, 2] for that file, while the new Rust reader expects global IDs starting at 3, so it discards the same three rows. Pure Python and pure Rust both return all six rows.
Please update the companion Python generator and raw reader (or normalize at the bridge) so both sides agree, and add row-tracking-without-DE coverage for all four planner/reader combinations. This is a current user-API result regression, not a request to preserve an unpublished split representation.
leaves12138
left a comment
There was a problem hiding this comment.
LGTM after re-reviewing 7596b12 together with the updated companion apache/paimon#10023 at 154af1a638b71efe1f96b0be79e4c03514ca43b7.
Both previously reported correctness issues are resolved with these paired heads: ordinary row-ID range reads return the expected rows, and row-tracked append chunk planning/reading now uses consistent global row-ID coordinates across Python and native paths. The exact eight-case reproducer (row tracking on/off x Python/native planning x Python/native reading) now passes without missing or duplicate rows.
Validation using the native extension built from this Rust head:
- 546 targeted companion PyPaimon tests passed.
- 89 Python-binding read tests passed.
- 18 additional mixed-runtime cases passed, covering append/data evolution, row tracking, deletion vectors, chunk sizes 1/3/5, worker assignment, partition-filter projection, and snapshot reads.
- flake8 passed on all changed companion Python files.
No remaining blocking issue found in the reviewed changes. Compatibility with intermediate, unpublished split representations is not being treated as a blocker.
What changed
IndexedSplit.row_rangesdata_filesorderwith_shard, independent of whether it is configured before or after chunk shufflefile_row_ranges,exact_merged_row_count, and metadata-only serialization pathCompatibility
Chunk splits now round-trip through the existing Java
SplitSerializerv1IndexedSplitformat. No unpublished native-only split fields or old paimon-rust compatibility path are required.Companion
PyPaimon integration and end-to-end coverage: apache/paimon#10023.
Verification
cargo clippy -p paimon -p pypaimon_rust --all-targets -- -D warningscargo fmt --all -- --check