feat: Introduce Deduplicator trait to unify mutable and immutable deduplication

[OussamaSaoudi]

🥞 Stacked PR

Use this link to review incremental changes.

• stack/dlr_add_rm_dedup [Files changed]
  • stack/dlr_immutable_add_rm_dedup [Files changed]
    • stack/dlr_executor [Files changed]
      • stack/dlr_api [Files changed]
        • stack/dlr_scan_state_serde [Files changed]

---

What changes are proposed in this pull request?

This PR introduces the Deduplicator trait which will unify Commit and Checkpoint deduplication under a single API. This is done to move the &mut HashSet<..> out of AddRemoveDedup and behind the Deduplicator trait. That way, we can reuse AddRemoveDedupVisitor in an immutable scenario.

How was this change tested?

[codecov]

Codecov Report

❌ Patch coverage is 80.55556% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.16%. Comparing base (743edec) to head (89e34bc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/log_replay/deduplicator.rs	82.35%	0 Missing and 3 partials ⚠️
kernel/src/expressions/mod.rs	66.66%	2 Missing ⚠️
kernel/src/scan/log_replay.rs	80.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1537   +/-   ##
=======================================
  Coverage   84.16%   84.16%           
=======================================
  Files         121      122    +1     
  Lines       34131    34108   -23     
  Branches    34131    34108   -23     
=======================================
- Hits        28726    28707   -19     
+ Misses       4017     4013    -4     
  Partials     1388     1388           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[OussamaSaoudi]

Note: these are moved out because (annoyingly), rust expects a proper type AddRemoveDedupVisitor::<FileActionDeduplicator>::ADD_PATH_INDEX

[OussamaSaoudi]

NOTE: this just moves it to the Deduplicator impl block.

[scovich]

Is the named lifetime actually used inside the impl block (guessing not, because trait impl)?
If not, can use anonymous lifetime here?

[OussamaSaoudi]

Note: these are moved here because (annoyingly), rust expects a proper type AddRemoveDedupVisitor::::ADD_PATH_INDEX

[OussamaSaoudi]

Here because of unnecessary visibility specifiers from trait Deduplicator.

[OussamaSaoudi]

imported to have access to the trait's check_record_and_seen

[scovich]

as _ can be used to clearly state that

[OussamaSaoudi]

Moved this directly from FileActionDeduplicator.

[scovich]

Does the key actually change? The trait name sounds generic enough, but then methods like extract_file_action sound quite specific to file actions and scan replay?

[OussamaSaoudi]

True. I'd initially wanted to use Deduplicator::Key to allow CheckpointDeduplicator use String. But String ended up complex enough to just keep it as FileActionKey.

I'll change to make the type fixed.

[scovich]

Should this be an associated constant? If we're ever doing checkpoint replay, we should be using a read-only deduplicator.

Tho that begs the question -- who actually cares (outside the trait impl itself) whether a given batch is from a commit or checkpoint? Is this all a vestige of the original code warts the Deduplicator trait tries to fix?

[OussamaSaoudi]

Yep this is still used a few places in AddRemoveDedupVisitor that uses is_log_batch, so it's vestigial until we move over the code in scan_metadata.

[scovich]

let Some(storage_type) = getters... else {
    return Ok(None);
};

let path_or_inline = ...?;
let offset = ...?;
Ok(Some(...))

[scovich]

LGTM, few nits

[nicklan]

i think i've ignored the serde stuff successfully and this part lgtm :)