Skip to content

Deprecate max statistics size properly #14188

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.

Sign up for GitHub

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

Jump to bottom
Merged
alamb merged 8 commits into from
Jan 25, 2025
Merged

Deprecate max statistics size properly #14188

alamb merged 8 commits into from
Jan 25, 2025

Conversation

@logan-keede
Copy link
Contributor

@logan-keede logan-keede commented Jan 18, 2025

Which issue does this PR close?

Closes #14172

Rationale for this change

Mark max_size_statistics deprecated

What changes are included in this PR?

  1. Mark the option deprecated in comments
  2. Make the field deprecated in the code

Are these changes tested?

builds properly, no functional changes

Are there any user-facing changes?

not yet.

@github-actions github-actions bot added the common Related to common crate label Jan 18, 2025
@logan-keede logan-keede mentioned this pull request Jan 18, 2025
Closed
alamb
alamb approved these changes Jan 18, 2025
View reviewed changes
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.

This looks good to me -- thank you @logan-keede

datafusion/common/src/config.rs

/// (writing) Sets max statistics size for any column. If NULL, uses
/// default parquet writer setting
/// max_statistics_size is deprecated, currently it is not being used
Copy link
Contributor

@alamb alamb Jan 18, 2025

Choose a reason for hiding this comment

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

👍

@github-actions github-actions bot added the proto Related to proto crate label Jan 19, 2025
@alamb
Copy link
Contributor

alamb commented Jan 19, 2025

I pushed another commit for #[allow(deprecated)]

@logan-keede
Copy link
Contributor Author

logan-keede commented Jan 19, 2025

I pushed another commit for #[allow(deprecated)]

Can I ask for the rationale is it just good practice or was it compiler giving warnings? (I am relatively new to rust.)

@alamb
Copy link
Contributor

alamb commented Jan 20, 2025

I pushed another commit for #[allow(deprecated)]

Can I ask for the rationale is it just good practice or was it compiler giving warnings? (I am relatively new to rust.)

The compiler was erroring. For example that is why this CI was failing

https://github.com/apache/datafusion/actions/runs/12847267558/job/35825203912

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 21, 2025
@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 24, 2025
@logan-keede
Copy link
Contributor Author

logan-keede commented Jan 24, 2025

I thought this was done, apparently not.

Please review the changes and let me know if we need to make more changes before merging this PR. @alamb
Thanks for you hard work,
Logan

@alamb alamb merged commit 18f14ab into apache:main Jan 25, 2025
26 checks passed
@alamb
Copy link
Contributor

alamb commented Jan 25, 2025

Thanks again @logan-keede

@rkrishn7 rkrishn7 mentioned this pull request Feb 5, 2025
Closed
This was referenced Feb 11, 2025
Closed
Closed
@carlosahs carlosahs mentioned this pull request Dec 14, 2025
Open
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

Assignees

No one assigned

Labels

common Related to common crate documentation Improvements or additions to documentation proto Related to proto crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deprecate datafusion.execution.parquet.max_statistics_size config option

2 participants

@logan-keede @alamb