Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ConfigOptions easier to work with #3886

Closed
Tracked by #4349
alamb opened this issue Oct 18, 2022 · 0 comments · Fixed by #4712
Closed
Tracked by #4349

Make ConfigOptions easier to work with #3886

alamb opened this issue Oct 18, 2022 · 0 comments · Fixed by #4712
Labels
enhancement New feature or request

Comments

@alamb
Copy link
Contributor

alamb commented Oct 18, 2022

Is your feature request related to a problem or challenge? Please describe what you are trying to do.
In order to use the ConfigOptions structure in more places, it was changed to be an Arc<RwLock<ConfigOptions>> by @waitingkuo in #3455 which is good

The usecase there was so that the configuration was owned by SessionContext but other parts could read it if necessary -- specifically, information_schema.df_settings / SHOW ALL initially.

I have been trying to consolidate these settings (see #3822) but find that working with ConfigOptions is somewhat of a pain (as you have to type Arc<RwLock> a bunch and then to update values you have to call read() and write()

Describe the solution you'd like

I would like to move the mutability handling into ConfigOption so this looks like

#[impl(Clone)]
struct ConfigOptions { 
  data: Arc<RwLock<...>>
}

And change all places in the code that use

Arc<RwLock<ConfigOptions>>

to

ConfigOptions

That would hide the details of shared ownership more nicely I think

Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.

Additional context
Came up here with @thinkharderdev : #3822 (comment)

@alamb alamb added the enhancement New feature or request label Oct 18, 2022
@alamb alamb changed the title Make ConfigOptions easier to configure / set Make ConfigOptions easier to work with Oct 18, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue Dec 22, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue Dec 22, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue Dec 22, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue Dec 22, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue Dec 22, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue Dec 22, 2022
tustvold added a commit to tustvold/arrow-datafusion that referenced this issue Dec 22, 2022
tustvold added a commit that referenced this issue Dec 23, 2022
* Don't share ConfigOptions (#3886)

* More tweaks

* Fix TestParquetFile

* Clippy

* Fix doctest

* Apply suggestions from code review

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
1 participant