Conversation
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM — clean optimization with solid test coverage. Two minor nits inline.
mihaibudiu
left a comment
There was a problem hiding this comment.
I cannot really ascertain the correctness, since I don't know the exact spec of Delta encodings, but here are some comments.
| } | ||
| } | ||
|
|
||
| // Process deletes before inserts. Semantically, delete actions are |
There was a problem hiding this comment.
I hope that's correct, whether it is or not depends on the definition of the delta tables data structures.
| // guarantee that actions occur in any particular order in the transaction log | ||
| // entry. | ||
| // | ||
| // Each pass hands `process_follow_action` the row-index delta for its |
There was a problem hiding this comment.
what would be very useful is a set of small golden delta files with all sorts of combinations where the output is known; we could use these golden files to test that this does the right thing. Is there a delta table test suite where we could steal these files from?
There was a problem hiding this comment.
can be a follow up, we test this already in python integration tests. but if required can get files from there
There was a problem hiding this comment.
you can ask claude to investigate whether there are such tests we could borrow
|
Please change "adapters" to "connectors", this is the agreed-upon name. Unfortunately renaming the crate is harder. |
| fn push_run(selectors: &mut Vec<RowSelector>, skip: bool, count: u64) { | ||
| /// Append a [`RowSelector`] for `count` rows to `selectors`, selecting or | ||
| /// skipping them per `select`, merging into the previous selector when it is | ||
| /// the same kind. A zero-length selector is a no-op. `select`/skip is parquet's |
There was a problem hiding this comment.
why is select backquoted and skip is not?
| // guarantee that actions occur in any particular order in the transaction log | ||
| // entry. | ||
| // | ||
| // Each pass hands `process_follow_action` the row-index delta for its |
There was a problem hiding this comment.
you can ask claude to investigate whether there are such tests we could borrow
mythical-fred
left a comment
There was a problem hiding this comment.
Follow-up LGTM. Prior comments look cleanly addressed. One tiny s/&/g mishap noted inline.
884f337 to
a157d1b
Compare
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. The DV-delta pairing is clean (data-change filter on both sides, new − old / old − new split, InBitmap read of only the changed rows, unpaired actions fall through to the whole-file path), the push_selector refactor drops the special-case gap handling with a nice proptest that pins keep ⊕ skip = 0..total_rows, and the docstring rename regression I flagged last round is fixed (both helper docs now correctly say Rows in ...).
The column-mapping addition is a good orthogonal extension: physical_read_schema renames only the fields whose ColumnMappingPhysicalName metadata differs from the logical name (so a table without column mapping is a strict no-op), advance_schema reloads the source table at each metaData commit so follow reads use the schema active when the commit was written, and project_physical_to_logical renames back after the DataFrame is built. Both name and id modes are exercised by tests; the mid-history / follow-from-version cases pin the "each commit resolves against its own schema" contract nicely, and _REPLAY_EXPECTED_ROWS documents the deliberate divergence from the snapshot expectation.
Nit (not blocking): the final squashed commit subject is still [adapters] read only the DV delta .... mihai asked for [connectors] on 2026-07-03; the PR title was updated but the commit subject was not. Fix on the way in.
A same-path Add+Remove in the transaction log is a deletion-vector rewrite: the Parquet bytes are unchanged and only the DV differs. Follow mode read the whole file on each side and let the circuit cancel the unchanged rows, so a K-row change ingested roughly 2N-K records. Pair such actions by path and decode both deletion vectors, then read only the rows the DV newly masks (deleted) or un-masks (restored). A K-row change now costs K records. The Python platform DV suite asserts total_input_records to catch the amplification Signed-off-by: Swanand Mulay <73115739+swanandx@users.noreply.github.com>
A same-path Add+Remove in the transaction log is a deletion-vector rewrite: the Parquet bytes are unchanged and only the DV differs. Follow mode read the whole file on each side and let the circuit cancel the unchanged rows, so a K-row change ingested roughly 2N-K records.
Pair such actions by path and decode both deletion vectors, then read only the rows the DV newly masks (deleted) or un-masks (restored). A K-row change now costs K records.
The Python platform DV suite asserts total_input_records to catch the amplification
Fix #6579
Describe Manual Test Plan
tested locally to verify it works
Checklist
Breaking Changes?
should not be breaking change