chore: Update SessionStateBuilder::with_default_features does not replace existing features

[irenjj]

Which issue does this PR close?

• Closes minor: SessionStateBuilder::with_default_features ergonomics #14899

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[alamb]

Thanks for working on this @irenjj -- I am not 100% sure about this implementation; let me know what you think

cc @milenkovicm

[alamb]

By putting these checks here I think the API will be quite confusing to use -- it means that calling with_expr_planners and the other methods below will only set the fields if there is no existing field set and silently ignore the argument if there is already planners specified

[alamb]

Thanks @irenjj

I thought the idea was that with_default_features would be additive to the existing set of features

That is, instead of replacing any existing factors/formats/etc this methods would append the default features

So instead of

        self.with_table_factories(SessionStateDefaults::default_table_factories())

Something more like

let mut table_factories = self.with_table_factories.take().unwrap_or_default();
table_factories.extend(SessionStateDefaults::default_table_factories())
self.table_factories = Some(table_factories)

🤔

[irenjj]

Thanks for the suggestion. I think the second approach is more suitable.

[milenkovicm]

I just wonder if we should provide with_default_features or Default::default implementation for this case as well? wdyt @alamb ?

[alamb]

Thank you @irenjj -- I think this makes a lot of sense to me

I just wonder if we should provide with_default_features or Default::default implementation for this case as well? wdyt @alamb ?

@milenkovicm I think the usecase of not automatically registering all the default features is to allow creating a SessionState without the defaults (e.g. if you don't want to defaults).

Since there is no method like SessionStateBuilder::clear I am not sure how else one would accomplish this usecase 🤔

[milenkovicm]

Thank you @irenjj -- I think this makes a lot of sense to me

I just wonder if we should provide with_default_features or Default::default implementation for this case as well? wdyt @alamb ?

@milenkovicm I think the usecase of not automatically registering all the default features is to allow creating a SessionState without the defaults (e.g. if you don't want to defaults).

Since there is no method like SessionStateBuilder::clear I am not sure how else one would accomplish this usecase 🤔

than makes sense, what I suggest is to keep all (new) methods as they are just provide new method which will create new with defaults. but I don't have strong opinion about it

[alamb]

I also made a small PR to try and clarify the docs:

• Improve SessionStateBuilder::new documentation #14980

[alamb]

Thank you @irenjj -- I think this makes a lot of sense to me

I just wonder if we should provide with_default_features or Default::default implementation for this case as well? wdyt @alamb ?

@milenkovicm I think the usecase of not automatically registering all the default features is to allow creating a SessionState without the defaults (e.g. if you don't want to defaults).
Since there is no method like SessionStateBuilder::clear I am not sure how else one would accomplish this usecase 🤔

than makes sense, what I suggest is to keep all (new) methods as they are just provide new method which will create new with defaults. but I don't have strong opinion about it

• Filed Add method to SessionStateBuilder that has all the defaut features #14981 to track

[alamb]

Thanks again @irenjj and @milenkovicm

It seems as if @shruti2522 already has a PR to implement the new_with_default_features 🎉

• Minor: add method SessionStateBuilder::new_with_default_features() #14998