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

Improve parquet WriterProperites and ReaderProperties docs #4392

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jun 9, 2023

Which issue does this PR close?

N/A

Rationale for this change

I was checking how the parquet writer was configured in IOx and it took me some digging to see what statistics were enabled by default. It would have been nice to avoid that digging

What changes are included in this PR?

  1. Improve the rustdocs by moving the property builder examples to the structure definitions they are used for
  2. Make the default values of the builder pub and document them

Are there any user-facing changes?

@github-actions github-actions bot added the parquet Changes to the parquet crate label Jun 9, 2023
@@ -15,55 +15,7 @@
// specific language governing permissions and limitations
// under the License.

//! [`WriterProperties`]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the examples to the WriterProperties and ReaderProperties so they would be easier to find

I think the module level docs now look pretty clear:

Screenshot 2023-06-09 at 11 07 30 AM

/// Default value for [`WriterProperties::dictionary_page_size_limit`]
pub const DEFAULT_DICTIONARY_PAGE_SIZE_LIMIT: usize = DEFAULT_PAGE_SIZE;
/// Default value for [`WriterProperties::statistics_enabled`]
pub const DEFAULT_STATISTICS_ENABLED: EnabledStatistics = EnabledStatistics::Page;
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 is what I was looking for, but since the value was not pub it wasn't in the rustdocs

I could have included the value in the text of WriterProperties::statistics_enabled but thought that this way was less likely to get out of sync

@tustvold tustvold merged commit 7bedb0a into apache:master Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
parquet Changes to the parquet crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants