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

docs: document SessionConfig #8771

Merged
merged 3 commits into from
Jan 11, 2024
Merged

Conversation

wjones127
Copy link
Member

Which issue does this PR close?

Closes #8770.

Rationale for this change

It took me a surprisingly long time to figure out how to set these configuration options.

What changes are included in this PR?

  • Link back to SessionConfig from various *Options structs so users can figure out where they are supposed to set them.
  • Added examples of how to set options in SessionConfig.
  • Brought up options() and options_mut() methods to beginning of the impl so that they are near the top of the documentation page. This makes them more visible. Also added simple examples for each.

Are these changes tested?

The documentation changes include doc-tested examples.

Are there any user-facing changes?

These are just documentation changes.

@@ -306,7 +322,7 @@ config_namespace! {
pub metadata_size_hint: Option<usize>, default = None

/// If true, filter expressions are be applied during the parquet decoding operation to
/// reduce the number of rows decoded
/// reduce the number of rows decoded. This optimization is sometimes called "late materialization".
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this because it was the option I was seeking out with the keyword late materialization.

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 11, 2024
@wjones127 wjones127 marked this pull request as ready for review January 11, 2024 05:43
@wjones127 wjones127 requested a review from alamb January 11, 2024 05:43
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @wjones127 -- I think this is a great improvement. I had some small suggestions but I think they could be done as a follow on PR or not at all.

I also rendered this locally and it was very nice 👨‍🍳 👌

Screenshot 2024-01-11 at 2 46 36 PM

/// Configuration options for Execution context
/// Configuration options for [`SessionContext`].
///
/// Can be passed to `SessionContext::with_config` to customize the configuration of DataFusion.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Can be passed to `SessionContext::with_config` to customize the configuration of DataFusion.
/// Can be passed to [`SessionContext::with_config`] to customize the configuration of DataFusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a manual link to this as well.

Also, discovered that SessionContext::with_config is deprecated so changed to new_with_config.

///
/// | Namespace | Config struct |
/// | --------- | ------------- |
/// | `datafusion.catalog` | [CatalogOptions][datafusion_common::config::CatalogOptions] |
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also mention add AggregateOptions ParquetOptions and SqlParserOptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

SqlParserOptions is already there, but added the other two.

@wjones127 wjones127 merged commit 910cc76 into apache:main Jan 11, 2024
23 checks passed
@wjones127 wjones127 deleted the doc-session-config branch January 11, 2024 21:07
@alamb
Copy link
Contributor

alamb commented Jan 12, 2024

Thanks again @wjones127

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document usage of SessionConfig
2 participants