refactor: Row tracking write cleanup #1291
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lbhm
commented
Sep 12, 2025
Comment on lines
+927
to
+934
| arrow::array::{ | ||
| Array, BooleanArray, Int32Array, Int64Array, ListArray, ListBuilder, MapBuilder, | ||
| MapFieldNames, RecordBatch, StringArray, StringBuilder, StructArray, | ||
| }, | ||
| arrow::datatypes::{DataType as ArrowDataType, Field, Schema}, | ||
| arrow::json::ReaderBuilder, |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1291 +/- ##
==========================================
- Coverage 84.12% 84.12% -0.01%
==========================================
Files 111 111
Lines 26893 26915 +22
Branches 26893 26915 +22
==========================================
+ Hits 22625 22641 +16
Misses 3161 3161
- Partials 1107 1113 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lbhm
commented
Sep 12, 2025
Comment on lines
+224
to
+225
| .chain(add_actions) | ||
| .chain(set_transaction_actions); |
kernel/src/transaction/mod.rs
Outdated
Comment on lines
385
to
386
| let commit_versions: Vec<i64> = | ||
| vec![commit_version.try_into()?; base_row_ids.len()]; |
Comment on lines
+360
to
+361
| let mut row_tracking_visitor = | ||
| RowTrackingVisitor::new(row_id_high_water_mark, Some(self.add_files_metadata.len())); |
Comment on lines
+84
to
+85
| /// Computed base row IDs of the visited actions, organized by batch | ||
| pub(crate) base_row_id_batches: Vec<Vec<i64>>, |
There was a problem hiding this comment.
is the overall memory usage the same? or are we 'keeping' more than we need to with this approach? old one was just tracking one layer up right?
There was a problem hiding this comment.
No, in both cases we need to materialize the entire Vec<Vec<i64>> because we need to know the final row ID row high watermark before we return the (lazily evaluated) add actions iterator. With this approach, we are even saving a clone() on the inner Vec (see lines 360/367 in the old transaction/mod.rs).
lbhm
force-pushed
the
row-tracking-write-cleanup
branch
from
82f9949 to
7617594
Compare
lbhm
force-pushed
the
row-tracking-write-cleanup
branch
from
7617594 to
6855979
Compare
lbhm
force-pushed
the
row-tracking-write-cleanup
branch
from
6855979 to
52102eb
Compare
aleksandarskrbic
pushed a commit
to aleksandarskrbic/delta-kernel-rs
that referenced
this pull request
Sep 21, 2025
## What changes are proposed in this pull request? This PR cleans up some nits as a follow-up to delta-io#1239. ## How was this change tested? Mostly refactoring. I added one more UT to verify the changes to the `RowTrackingVisitor`. Co-authored-by: Zach Schuermann <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
This PR cleans up some nits as a follow-up to #1239.
How was this change tested?
Mostly refactoring. I added one more UT to verify the changes to the
RowTrackingVisitor.