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

Minor: improve ParquetOpener docs #12456

Merged
merged 1 commit into from
Sep 15, 2024
Merged

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Sep 13, 2024

Which issue does this PR close?

Part of #4028

Rationale for this change

While working with @itsjunetime on #12135 we spent a while reviewing the code in ParquetOpener and I realized that the documentation could be improved

What changes are included in this PR?

Improve the documentation

Are these changes tested?

By CI

Are there any user-facing changes?

No functional changes, just docs

@github-actions github-actions bot added the core Core DataFusion crate label Sep 13, 2024
pub partition_index: usize,
/// Column indexes in `table_schema` needed by the query
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This in particular was unclear when @itsjunetime and I read the code (were the indexes in terms of file_schema or table_schema?) and was the impetus for this PR

Copy link
Contributor

@appletreeisyellow appletreeisyellow 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 @alamb! The descriptions are helpful, especially those for bool values!

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

Nice, WDYT about enabling the missing_docs lint so every pub will force forced to be documented ?

@comphead
Copy link
Contributor

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e486601506c2493123d2b4d9cca24727
here is the link to playground, the lint will require crate documentation and public entities

@alamb
Copy link
Contributor Author

alamb commented Sep 15, 2024

Nice, WDYT about enabling the missing_docs lint so every pub will force forced to be documented ?

I personally think it is a good idea, though I predict that if we do that we'll still end up with functions that are like "ParquetOpener: opens parquet" any other similar documentation that isn't particularly helpful.

But it might encourage enough that is better than no documentation 👍

@alamb alamb added the documentation Improvements or additions to documentation label Sep 15, 2024
@alamb alamb merged commit 6519f8e into apache:main Sep 15, 2024
24 checks passed
@alamb
Copy link
Contributor Author

alamb commented Sep 15, 2024

Thanks for the review @appletreeisyellow and @comphead

@alamb alamb deleted the alamb/parquet_docs branch September 15, 2024 11:59
@comphead
Copy link
Contributor

I personally think it is a good idea, though I predict that if we do that we'll still end up with functions that are like "ParquetOpener: opens parquet" any other similar documentation that isn't particularly helpful.

But it might encourage enough that is better than no documentation 👍

I'll try to play on some small crate first, enabling the documentation and putting #[allow(missing_docs)] whenever it complains, so later it would be easier to find all undocumented methods and make contributions

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants