Repository navigation
[core] Read single-run primary-key sections without sort merge - #1164
JeonDaehong wants to merge 1 commit into
Conversation
|
@JingsongLi @leaves12138 could you take a look when you have time? I'm looking at running Paimon with the Rust reader for work, so read-path performance matters to me, and I'd like to keep improving it here. |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. Implementation: FINDINGS at c8b5893. Avoiding unnecessary merging has end-to-end read value, but the shortcut regresses reads of historical native retract files. Local verification: 55 existing KV/single-run/user-sequence/first-row/DV tests pass. An additional public-API regression test writes actual Parquet, commits historical metadata, and reads a two-file nonraw split through the ordinary table planner: it fails on this head (returns [1, 2, 3]) and passes unchanged on base 46e6871 (returns [1, 2]). Details inline. Formatting and clean integration with current main were also checked.
| && merge_group.iter().all(|run| { | ||
| run.files | ||
| .iter() | ||
| .all(|file| file.file.delete_row_count == Some(0)) |
There was a problem hiding this comment.
[P2] Preserve retract filtering for historical native files
delete_row_count == Some(0) does not prove these files contain only add rows. Before 13e370a (#318, May 20), the Rust KV writer accepted _VALUE_KIND but persisted Some(0) unconditionally; normal cross-partition writes also generated DELETE batches. Those files have no writer-version guard here. I reproduced a full table read with two disjoint files in one nonraw split: file A has INSERT keys 1 and 2; file B has a DELETE for key 3 with that historical metadata. Base 46e6871 returns [1, 2], while this head returns [1, 2, 3], exposing the deleted row. The test asserts that the planner marks this two-file split nonraw, so this is an expansion beyond the pre-existing single-file raw path. Please preserve a cheap RowKind filter on the direct path, or restrict the optimization to files whose add-only invariant can actually be verified, and cover this legacy case.
There was a problem hiding this comment.
Good catch, thanks. Fixed in 476312c: the shortcut no longer relies on delete_row_count. Each unmerged batch drops retract rows the same way the merge path handles a lone version of a key. First-row without ignore-delete still errors on them. single_run_read_drops_retracts_hidden_by_legacy_metadata covers your two-file case for deduplicate, first-row and first-row + ignore-delete. Without the filter it returns [1, 2, 3].
While running CI I also found that read_with_merge_function took the shortcut, which dropped -D rows from $audit_log (test_audit_log_system_table_keeps_row_kinds_and_sequence_numbers). The shortcut now applies only to read, so custom merge functions always merge.
Rebased onto current main and re-measured on one instance, alternating: main 157 ms vs this head 60 ms. Ranges don't overlap.
A section with one sorted run of files that hold no retract rows, under the deduplicate or first-row engine, has no key to merge: the writer keeps keys unique within a file and the run's files don't overlap. Planning already reads such a split raw when it has one file. KeyValueFileReader now streams these sections directly instead of running the per-row merge, so small non-overlapping files packed into one split no longer pay for it.
c8b5893 to
476312c
Compare
Purpose
Linked issue: close #1163
Brief change log
KeyValueFileReader::readskips the sort merge for a section with one sorted run under the deduplicate or first-row engine. The run's files don't overlap and keys are unique within a file, so every key has a single version. Each batch still drops retract rows the way the merge handles a lone version, and first-row withoutignore-deleterejects them. That's needed because older Rust writers recordeddelete_row_count = 0for files holding DELETE rows.read_with_merge_function(audit log) always merges. Other sections merge as before.Disjoint level-0 files packed into one split, 2M rows: 157 ms → 60 ms on current main (alternating on one instance, ranges don't overlap). Overlapping sections take the merge path unchanged.
Tests
pk_single_run_read_test: three disjoint files in one split read back once each, with a value-only projection and a residual filter; overlapping one of them still merges to the newer value.single_run_read_drops_retracts_hidden_by_legacy_metadata: a two-file run whose second file holds a DELETE withdelete_row_count = 0reads[1, 2]under deduplicate and first-row withignore-delete, and errors under first-row.dv_merge_on_read_loads_deletion_vectors_per_fileasserted the first batch had 128 rows, the merge's output size. Its delete-free files now stream unmerged, so it checks the batch is non-empty; the lazy DV loading it covers is unchanged.Ran the ci.yml steps locally (fmt, clippy, msrv, unit, rest-server, integration, datafusion, python, go).
API and Format
None.
Documentation
None.