Provide a mechanism for setting the version Option

[rtyler]

The SnapshotBuilder is a reasonable improvement IMHO, but the fluent
style because challenging when the caller has an Option<u64> as well.

I have been fond of the AWS SDK fluent
builders
which provide a account_id(id) and set_account_id(Option<id>) style
builders which give callers flexibility.

Instead of:

let mut builder = Snapshot::builder(url);
builder  = match version.as_ref() {
  Some(version) => builder.at_version(version),
  None => builder,
};
let snapshot = builder.build(engine.as_ref();

Callers can instead:

let snapshot =
Snapshot::builder(url).set_version(version).build(engine.as_ref());

[codecov]

Codecov Report

❌ Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.63%. Comparing base (447767e) to head (5ea3f0a).

Files with missing lines	Patch %	Lines
kernel/src/snapshot/builder.rs	0.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1236      +/-   ##
==========================================
- Coverage   83.65%   83.63%   -0.03%     
==========================================
  Files         106      106              
  Lines       24966    24970       +4     
  Branches    24966    24970       +4     
==========================================
- Hits        20886    20883       -3     
- Misses       3046     3051       +5     
- Partials     1034     1036       +2     

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

[rtyler]

@roeap FYI I hit this papercut when trying to integrate the latest main against delta-rs 😞

[roeap]

@rtyler - i think for primitive types there is also the option to do

set_version(&self, version: impl Into<Option<Version>>) ...

which allows to pass either an option or the value.

What I wanted to look into - though not relevant for this specific case - if there is a way to get this also working when the inner type is something like Into<String>.

[zachschuermann]

What about at_version_opt (seems to match existing API better)? Or perhaps just unifying it all with at_version to take impl Into<Option> like @roeap says?

[zachschuermann]

we had a decent bit of discussion on naming already - i'll let others chime in cc @nicklan @scovich @roeap

[scovich]

IMO the real problem is letting people set the version at all, as if that were somehow orthogonal to all the other information that could be passed (e.g. set version to 10 but pass P&M for latest version 15). Which points to having a builder with two different constructors -- one for a latest-version builder and one for an as-of-version builder. But that doesn't actually prevent the caller from passing latest P&M to a time travel builder, so meh.

Meanwhile, I'm totally fine with impl Into<Option>, we've used that trick elsewhere -- notably when setting start and end versions for CDF -- for the exact same papercut-avoidance that @rtyler aims for here.

We can adjust the API further if/when we decide to fix the larger ergonomics/footgun issue (which first requires having some idea of how to fix it, still TBD).

[zachschuermann]

yea and to add the current proposal for the "different kind of versions": we will have a builder which bifurcates into two different types based on if you do an at_version(version) or with_metadata(P, M, version). see #1189 (comment)

[scovich]

What I wanted to look into - though not relevant for this specific case - if there is a way to get this also working when the inner type is something like Into<String>.

I think you can do arg: impl Into<Option<impl Into<String>> -- the recipient has to do arg.into().map(Into::into). But from what I remember it can also be a pain in the neck for type inference?

[roeap]

I think you can do arg: impl Into<Option<impl Into> -- the recipient has to do arg.into().map(Into::into). But from what I remember it can also be a pain in the neck for type inference?

this sometimes works:

struct MyBuilder {
      field1: Option<String>,
}

impl MyBuilder {
    fn with_field1<T: Into<String>>(mut self, field1: impl Into<Option<T>>) -> Self {
        self.field1 = field1.into().map(Into::into);
        self
    }
  }

But as you mention causes issues at the call site, even this does not work for me then ..

fn call1(arg: Option<String>) {
    let mut builder = MyBuilder { field1: None };
    let builder = builder.with_field1(arg);
}