feat: during LakeFS file operations, skip merge when 0 changes

[smeyerre]

Description

If a branch does not have any file changes, we no longer merge the branch to avoid empty commits.

Related Issue(s)

• closes lakefs: during file operations, skip merge when 0 changes #3190

Documentation

N/A

[github-actions]

ACTION NEEDED

delta-rs follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

[smeyerre]

First time contributing here so I'll include my manual testing process here in case I missed anything!

Manual Testing

Testing was performed using a real LakeFS instance:

1. Environment Setup

   • Started LakeFS using Docker Compose
   • Built delta-rs with the implemented changes

2. Test Steps

   • Created a Delta table with initial data (verified first commit in LakeFS UI)
   • Added more data to the table (verified second commit in LakeFS UI)
   • Performed a no-op operation (repair with dry_run=True)
   • Verified that no empty commit was created (commit count remained at 2)

3. Results

   • Confirmed that real operations (table creation, data addition) still create commits
   • Confirmed that no-op operations no longer create empty commits
   • The fix works correctly without affecting normal Delta Lake operations

[ion-elgreco]

I had a quick glance and it looks good! You should change the test though, a repair dry run doesn trigger pre execute and post execute since it returns the metrics before that.

But an easier way to trigger a file operation lakefs commit is vacuum

[smeyerre]

Ahh good to know, thanks I'll make that change!

[codecov]

Codecov Report

Attention: Patch coverage is 76.52174% with 27 lines in your changes missing coverage. Please review.

Project coverage is 72.20%. Comparing base (6437e7a) to head (aee2e6c).

Files with missing lines	Patch %	Lines
crates/lakefs/src/client.rs	81.08%	12 Missing and 2 partials ⚠️
crates/lakefs/src/logstore.rs	56.66%	8 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3346      +/-   ##
==========================================
- Coverage   72.20%   72.20%   -0.01%     
==========================================
  Files         145      145              
  Lines       45890    45986      +96     
  Branches    45890    45986      +96     
==========================================
+ Hits        33135    33202      +67     
- Misses      10667    10679      +12     
- Partials     2088     2105      +17     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[smeyerre]

Ok I'm realizing I missed a failing test earlier. It seems that when creating checkpoints, the checkpoint files aren't reflected by the LakeFS diff API.

I can think of a couple ways to go about handling this:

1. Add something like a force_merge parameter to commit_merge that gets passed when doing checkpoint operations
2. Add a more rigorous check for changes in has_changes by doing something like looking for checkpoint files as well

Option 1 seems a little bit weird to me so I'd be more inclined to do 2 but open to feedback on this (also I could have easily missed a better option).

[ion-elgreco]

Ok I'm realizing I missed a failing test earlier. It seems that when creating checkpoints, the checkpoint files aren't reflected by the LakeFS diff API.

I can think of a couple ways to go about handling this:

1. Add something like a force_merge parameter to commit_merge that gets passed when doing checkpoint operations
2. Add a more rigorous check for changes in has_changes by doing something like looking for checkpoint files as well

Option 1 seems a little bit weird to me so I'd be more inclined to do 2 but open to feedback on this (also I could have easily missed a better option).

What does the diff API return here? In theory it should show any change

[smeyerre]

Ok I'm realizing I missed a failing test earlier. It seems that when creating checkpoints, the checkpoint files aren't reflected by the LakeFS diff API.
I can think of a couple ways to go about handling this:

1. Add something like a force_merge parameter to commit_merge that gets passed when doing checkpoint operations
2. Add a more rigorous check for changes in has_changes by doing something like looking for checkpoint files as well

Option 1 seems a little bit weird to me so I'd be more inclined to do 2 but open to feedback on this (also I could have easily missed a better option).

What does the diff API return here? In theory it should show any change

Apologies, I've been out of town the last week and a half.

So in my manual testing it seems like the LakeFS diff API actually returns empty results when a checkpoint has been made. Is that not expected?

[ion-elgreco]

Ok I'm realizing I missed a failing test earlier. It seems that when creating checkpoints, the checkpoint files aren't reflected by the LakeFS diff API.
I can think of a couple ways to go about handling this:

1. Add something like a force_merge parameter to commit_merge that gets passed when doing checkpoint operations
2. Add a more rigorous check for changes in has_changes by doing something like looking for checkpoint files as well

Option 1 seems a little bit weird to me so I'd be more inclined to do 2 but open to feedback on this (also I could have easily missed a better option).

What does the diff API return here? In theory it should show any change

Apologies, I've been out of town the last week and a half.

So in my manual testing it seems like the LakeFS diff API actually returns empty results when a checkpoint has been made. Is that not expected?

Are you sure the test is correct? I remember we used the diff api before doing a merge on the python side and that worked

[smeyerre]

Ok I'm realizing I missed a failing test earlier. It seems that when creating checkpoints, the checkpoint files aren't reflected by the LakeFS diff API.
I can think of a couple ways to go about handling this:

1. Add something like a force_merge parameter to commit_merge that gets passed when doing checkpoint operations
2. Add a more rigorous check for changes in has_changes by doing something like looking for checkpoint files as well

Option 1 seems a little bit weird to me so I'd be more inclined to do 2 but open to feedback on this (also I could have easily missed a better option).

What does the diff API return here? In theory it should show any change

Apologies, I've been out of town the last week and a half.
So in my manual testing it seems like the LakeFS diff API actually returns empty results when a checkpoint has been made. Is that not expected?

Are you sure the test is correct? I remember we used the diff api before doing a merge on the python side and that worked

Ah ok I think I fixed the issue. It seems like I had the branch parameters to the diff API call reversed which messed with the checkpoint diffs specifically. The tests are all passing now, and my manual tests worked as well.

[ion-elgreco]

@smeyerre great! Can you fix the formatting? Then we're good to go 💯

[smeyerre]

@smeyerre great! Can you fix the formatting? Then we're good to go 💯

Oh oops, ok that should be fixed now, unit/formatting tests are passing locally.

[ion-elgreco]

@smeyerre don't worry about the other test failures, there is a breaking change in the arrow release, I will merge it once I've addressed it in main

[smeyerre]

@smeyerre don't worry about the other test failures, there is a breaking change in the arrow release, I will merge it once I've addressed it in main

Awesome thanks!

[ion-elgreco]

Thanks for adding this optimization!! <3