feat: Introduce Deduplicator trait to unify mutable and immutable deduplication #1537
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
This was referenced Dec 3, 2025
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1537 +/- ##
==========================================
+ Coverage 84.77% 84.89% +0.12%
==========================================
Files 126 130 +4
Lines 35679 36372 +693
Branches 35679 36372 +693
==========================================
+ Hits 30246 30878 +632
- Misses 3965 3987 +22
- Partials 1468 1507 +39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
github-actions
bot
added
the
breaking-change
OussamaSaoudi
changed the title
OussamaSaoudi
commented
Dec 4, 2025
Comment on lines
-233
to
-109
| // These index positions correspond to the order of columns defined in | ||
| // `selected_column_names_and_types()` | ||
| const ADD_PATH_INDEX: usize = 0; // Position of "add.path" in getters | ||
| const ADD_PARTITION_VALUES_INDEX: usize = 1; // Position of "add.partitionValues" in getters | ||
| const ADD_DV_START_INDEX: usize = 2; // Start position of add deletion vector columns | ||
| const BASE_ROW_ID_INDEX: usize = 5; // Position of add.baseRowId in getters | ||
| const REMOVE_PATH_INDEX: usize = 6; // Position of "remove.path" in getters | ||
| const REMOVE_DV_START_INDEX: usize = 7; // Start position of remove deletion vector columns |
Collaborator
Author
There was a problem hiding this comment.
Note: these are moved out because (annoyingly), rust expects a proper type AddRemoveDedupVisitor::<FileActionDeduplicator>::ADD_PATH_INDEX
OussamaSaoudi
commented
Dec 4, 2025
| } | ||
| } | ||
|
|
||
| impl<'seen> Deduplicator for FileActionDeduplicator<'seen> { |
Collaborator
Author
There was a problem hiding this comment.
NOTE: this just moves it to the Deduplicator impl block.
OussamaSaoudi
commented
Dec 4, 2025
Comment on lines
+85
to
+91
| const ADD_PATH_INDEX: usize = 0; // Position of "add.path" in getters | ||
| const ADD_PARTITION_VALUES_INDEX: usize = 1; // Position of "add.partitionValues" in getters | ||
| const ADD_DV_START_INDEX: usize = 2; // Start position of add deletion vector columns | ||
| const BASE_ROW_ID_INDEX: usize = 5; // Position of add.baseRowId in getters | ||
| const REMOVE_PATH_INDEX: usize = 6; // Position of "remove.path" in getters | ||
| const REMOVE_DV_START_INDEX: usize = 7; // Start position of remove deletion vector columns |
Collaborator
Author
There was a problem hiding this comment.
Note: these are moved here because (annoyingly), rust expects a proper type AddRemoveDedupVisitor::::ADD_PATH_INDEX
OussamaSaoudi
commented
Dec 4, 2025
| /// - `Ok(None)`: When no file action is found | ||
| /// - `Err(...)`: On any error during extraction | ||
| pub(crate) fn extract_file_action<'a>( | ||
| fn extract_file_action<'a>( |
Collaborator
Author
There was a problem hiding this comment.
Here because of unnecessary visibility specifiers from trait Deduplicator.
OussamaSaoudi
commented
Dec 4, 2025
| //! actions selected | ||
| //! | ||
| use crate::engine_data::{FilteredEngineData, GetData, RowVisitor, TypedGetData as _}; | ||
| use crate::log_replay::deduplicator::Deduplicator; |
Collaborator
Author
There was a problem hiding this comment.
imported to have access to the trait's check_record_and_seen
Draft
OussamaSaoudi
force-pushed
the
stack/dlr_add_rm_dedup
branch
from
433c0aa to
36651ea
Compare
Draft
OussamaSaoudi
force-pushed
the
stack/dlr_add_rm_dedup
branch
from
36651ea to
8c284ae
Compare
OussamaSaoudi
commented
Dec 8, 2025
| /// - `dv_start_index` retrieves the storage type (`deletionVector.storageType`). | ||
| /// - `dv_start_index + 1` retrieves the path or inline deletion vector (`deletionVector.pathOrInlineDv`). | ||
| /// - `dv_start_index + 2` retrieves the optional offset (`deletionVector.offset`). | ||
| fn extract_dv_unique_id<'a>( |
Collaborator
Author
There was a problem hiding this comment.
Moved this directly from FileActionDeduplicator.
OussamaSaoudi
force-pushed
the
stack/dlr_add_rm_dedup
branch
from
d71a38d to
9312426
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
🥞 Stacked PR
Use this link to review incremental changes.
What changes are proposed in this pull request?
This PR introduces the
Deduplicatortrait which will unify Commit and Checkpoint deduplication under a single API. This is done to move the&mut HashSet<..>out ofAddRemoveDedupand behind theDeduplicatortrait. That way, we can reuseAddRemoveDedupVisitorin an immutable scenario.How was this change tested?