chore: set max_retries in CommitProperties

[helanto]

Description

Adds a with_max_retries in CommitProperties. Allow for clients to control the maximum number of times a commit will be retried before failing.

Related Issue(s)

Documentation

[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.

[ion-elgreco]

This needs to be exposed for python users as well

[helanto]

This needs to be exposed for python users as well

Not super familiar with how Python binding works. Made an attempt to expose the max_commit_retries in Python API. Let me know what you think. @ion-elgreco

[ion-elgreco]

@helanto Hmm perhaps we can introduce a CommitProperties class, where you have custom_metadata and commit_retries as variables, this would be a breaking change, so we need to warn users that custom_metadata should be passed into CommitProperties

[helanto]

@helanto Hmm perhaps we can introduce a CommitProperties class, where you have custom_metadata and commit_retries as variables, this would be a breaking change, so we need to warn users that custom_metadata should be passed into CommitProperties

I believe I made the change. How do we warn users @ion-elgreco ?

[ion-elgreco]

@helanto Hmm perhaps we can introduce a CommitProperties class, where you have custom_metadata and commit_retries as variables, this would be a breaking change, so we need to warn users that custom_metadata should be passed into CommitProperties

I believe I made the change. How do we warn users @ion-elgreco ?

You can check in other places of the code base, where we show a warn when a parameter is passed that is deprecated

[helanto]

You can check in other places of the code base, where we show a warn when a parameter is passed that is deprecated

This means that we keep both parameters around. Do you have something in mind along the following lines?

def operation(
    ....,
    custom_metadata: Optional[Dict[str, str]] = None,  # We keep this around for compatibility reasons.
    commit_properties: OptionalCommit[Properties] = None,
):
    if custom_metadata is not None:
        warn("Please provide custom_metadata as part of commit_properties").
    ...

[ion-elgreco]

You can check in other places of the code base, where we show a warn when a parameter is passed that is deprecated

This means that we keep both parameters around. Do you have something in mind along the following lines?

def operation(
    ....,
    custom_metadata: Optional[Dict[str, str]] = None,  # We keep this around for compatibility reasons.
    commit_properties: OptionalCommit[Properties] = None,
):
    if custom_metadata is not None:
        warn("Please provide custom_metadata as part of commit_properties").
    ...

Yess, and then custom_metadata can be dropped in later versions

[ion-elgreco]

@helanto do you have time to make that change? Because then we can merge : )

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 61 lines in your changes missing coverage. Please review.

Project coverage is 72.66%. Comparing base (8ba4fe0) to head (289fd00).

Files with missing lines	Patch %	Lines
python/src/lib.rs	0.00%	55 Missing ⚠️
crates/core/src/operations/transaction/mod.rs	0.00%	4 Missing ⚠️
python/src/merge.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2826      +/-   ##
==========================================
- Coverage   72.67%   72.66%   -0.02%     
==========================================
  Files         131      131              
  Lines       39957    39962       +5     
  Branches    39957    39962       +5     
==========================================
- Hits        29040    29037       -3     
- Misses       9059     9064       +5     
- Partials     1858     1861       +3     

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

[helanto]

@helanto do you have time to make that change? Because then we can merge : )

Yes I will work on it today!

[helanto]

@helanto do you have time to make that change? Because then we can merge : )

Yes I will work on it today!

I believe I made the changes if you can take a look @ion-elgreco

[ion-elgreco]

@helanto just one more thing, commit_metadata in the pyo3 functions can be removed, and in Python if custom metadata is still passed, you can wrap it into CommitPropeti e

[helanto]

@helanto just one more thing, commit_metadata in the pyo3 functions can be removed, and in Python if custom metadata is still passed, you can wrap it into CommitPropeti e

I look at pyo3 function definitions and they don't have any commit_metadata argument ? Do you mean custom_metadata ? The pyo3 functions look like the following:

#[pyo3(signature = ...)]
pub fn some_function(
        ...,
        custom_metadata: Option<HashMap<String, String>>,
        max_commit_retries: Option<usize>,
)

I can create a pyo3 class that wraps custom_metadata and max_commit_retries, similar to PyPostCommitHookProperties here. At the moment the CommitProperties only exist as a publicly facing Python class, it's not a pyo3 class. @ion-elgreco

[ion-elgreco]

Yeah I meant custom_metadata. We need a rust struct which has a derive indeed

[helanto]

Yeah I meant custom_metadata. We need a rust struct which has a derive indeed

Now it should be good! Let me know what you think @ion-elgreco

[ion-elgreco]

Thankss!!