Skip to content

Conversation

@ayushdg
Copy link
Collaborator

@ayushdg ayushdg commented Feb 4, 2022

This PR removes ConfigContainer and utilities to modify them as added in #286 in favor of using upstream Dask's config module and the flexibility it provides.

Key Changes:

  • Config options are no longer schema specific. Users can either modify/provide a set of configurations that apply globally across all operations or specify config options on a per query basis.
  • The keywords used for different configuration options are being renamed to be more in line with the dask config pattern. All config options for dask-sql start with the prefix sql.
    • eg: dask.groupby.aggregate.split_out renamed to sql.groupby.split_out

Functionality Enabled:

  • Users can provide dask-sql configurations in yaml files, as environment variables or any other way dask supports and will become a part of the global dask config which can be consumed by any Dask-project that uses dask.config.
  • Users can specify configuration options on a per-query basis by using context.sql({"query_string"},config_options={options}).

Checklist:

  • Add new config features
  • Remove the usage of ConfigContainer in favor of dask_sql.config
  • Add tests for config
  • Update setup to ensure the yaml files are packaged properly

@codecov-commenter
Copy link

codecov-commenter commented Feb 23, 2022

Codecov Report

Merging #392 (58eada8) into main (eee8c65) will increase coverage by 0.23%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #392      +/-   ##
==========================================
+ Coverage   88.94%   89.17%   +0.23%     
==========================================
  Files          68       69       +1     
  Lines        3337     3327      -10     
  Branches      657      654       -3     
==========================================
- Hits         2968     2967       -1     
+ Misses        297      287      -10     
- Partials       72       73       +1     
Impacted Files Coverage Δ
dask_sql/datacontainer.py 96.96% <ø> (+3.16%) ⬆️
dask_sql/__init__.py 100.00% <100.00%> (ø)
dask_sql/config.py 100.00% <100.00%> (ø)
dask_sql/context.py 100.00% <100.00%> (ø)
dask_sql/physical/rel/logical/aggregate.py 94.77% <100.00%> (-0.04%) ⬇️
dask_sql/_version.py 34.00% <0.00%> (+1.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eee8c65...58eada8. Read the comment docs.

@ayushdg ayushdg marked this pull request as ready for review February 28, 2022 22:53
@ayushdg ayushdg changed the title [WIP] Refactor ConfigContainer to use dask config [Review] Refactor ConfigContainer to use dask config Feb 28, 2022
Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice 🙂 especially like the configuration reference page in the docs; just some questions related to the handling of Context.set_config and then I think this should be good to merge.

Also going to merge in the latest changes from #406 to see if that resolves the API reference issues in the docs build for this PR.

@ayushdg
Copy link
Collaborator Author

ayushdg commented Mar 7, 2022

@charlesbluca Should be ready for another round of reviews.

Copy link
Collaborator

@charlesbluca charlesbluca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; thanks for the work here @ayushdg 😄

@charlesbluca charlesbluca merged commit 0372ebc into dask-contrib:main Mar 7, 2022
@ayushdg ayushdg deleted the enh-dask-sql-config branch February 6, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants