fix: validate parquet statistics config by zhangxinyao88 · Pull Request #24642 · apache/datafusion

zhangxinyao88 · 2026-08-24T23:22:54Z

Which issue does this PR close?

Part of The config values aren't checked #17498.

Rationale for this change

datafusion.execution.parquet.statistics_enabled accepts arbitrary strings. Invalid values silently fall back to the Parquet writer default instead of failing when set.

What changes are included in this PR?

This adds a typed config enum for none, chunk, and page. It validates values at config and protobuf boundaries and passes the validated value to the Parquet writer.

Are these changes tested?

Yes. Unit, protobuf round-trip, and SQL logic tests were added. The full lint and extended workspace test suites pass.

Are there any user-facing changes?

Invalid values now fail at SET time. Valid values remain case-insensitive and are displayed in lowercase.

This changes ParquetOptions.statistics_enabled from Option<String> to Option<DFParquetStatistics>, so the PR may need the api change label.

kosiew

@zhangxinyao88,

Thanks for working on this. Moving the Parquet statistics setting to a validated enum is a nice improvement and makes invalid configuration fail much earlier.

I found two issues that I think need to be addressed before merging. The new config test currently fails to compile without the parquet feature, and a failed SET after RESET can unexpectedly mutate the configuration to Some(Page).

I also left one non-blocking suggestion to add coverage for the proto-models decoding path.

kosiew · 2026-08-28T08:59:54Z

        );
    }

+    #[test]


Could you gate this test with #[cfg(feature = \"parquet\")], or make the assert_contains! import unconditional? Right now the test itself is always compiled, but the macro import is only available with the parquet feature. As a result, the default-feature test build fails with cannot find macro assert_contains in this scope.

Fixed, thanks. I gated the parquet validation test on the parquet feature, so the default-feature build no longer hits the parquet-only assertion macro.

kosiew · 2026-08-28T09:00:54Z

        /// These values are not case sensitive. If NULL, uses
        /// default parquet writer setting
-        pub statistics_enabled: Option<String>, transform = str::to_lowercase, default = Some("page".into())
+        pub statistics_enabled: Option<DFParquetStatistics>, default = Some(DFParquetStatistics::Page)


There is a subtle state mutation here when this option is None. The blanket Option<F>::set inserts DFParquetStatistics::default() before trying to parse the new value. This means that after RESET datafusion.execution.parquet.statistics_enabled, running SET ... = 'invalid' correctly returns an error, but also changes the setting from None to Some(Page).

Could we use the parse-then-assign pattern used by Option<MaxRowGroupBytes> so a failed SET leaves the existing configuration unchanged? It would also be good to add a regression assertion covering RESET followed by an invalid SET.

Fixed, thanks. The option now parses before assigning, so an invalid SET leaves an unset value untouched. RESET restores the configured default, Page, so the regression test explicitly starts from an unset value before trying the invalid update.

kosiew · 2026-08-28T09:01:50Z

            dictionary_page_size_limit: proto.dictionary_page_size_limit as usize,
-            statistics_enabled: proto.statistics_enabled_opt.as_ref().map(
-                |opt| match opt {
+            statistics_enabled: proto


Non-blocking suggestion: could we add a direct negative test for this decoder, similar to the invalid-value test added in proto-common? Since this is a separate deserialization boundary, having a test here would make sure malformed wire values continue to be rejected if the implementations evolve independently.

Added a negative decoder test for an invalid parquet statistics value as well.

github-actions · 2026-08-28T09:56:44Z

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

     Cloning apache/main
    Building datafusion-common v55.0.0 (current)
       Built [  29.904s] (current)
     Parsing datafusion-common v55.0.0 (current)
      Parsed [   0.056s] (current)
    Building datafusion-common v55.0.0 (baseline)
       Built [  26.729s] (baseline)
     Parsing datafusion-common v55.0.0 (baseline)
      Parsed [   0.054s] (baseline)
    Checking datafusion-common v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.836s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  59.572s] datafusion-common
    Building datafusion-datasource-parquet v55.0.0 (current)
       Built [  39.583s] (current)
     Parsing datafusion-datasource-parquet v55.0.0 (current)
      Parsed [   0.030s] (current)
    Building datafusion-datasource-parquet v55.0.0 (baseline)
       Built [  40.247s] (baseline)
     Parsing datafusion-datasource-parquet v55.0.0 (baseline)
      Parsed [   0.036s] (baseline)
    Checking datafusion-datasource-parquet v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.177s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  81.036s] datafusion-datasource-parquet
    Building datafusion-proto-common v55.0.0 (current)
       Built [  17.932s] (current)
     Parsing datafusion-proto-common v55.0.0 (current)
      Parsed [   0.041s] (current)
    Building datafusion-proto-common v55.0.0 (baseline)
       Built [  17.780s] (baseline)
     Parsing datafusion-proto-common v55.0.0 (baseline)
      Parsed [   0.042s] (baseline)
    Checking datafusion-proto-common v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   1.282s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [  38.021s] datafusion-proto-common
    Building datafusion-proto-models v55.0.0 (current)
       Built [  20.309s] (current)
     Parsing datafusion-proto-models v55.0.0 (current)
      Parsed [   0.110s] (current)
    Building datafusion-proto-models v55.0.0 (baseline)
       Built [  20.394s] (baseline)
     Parsing datafusion-proto-models v55.0.0 (baseline)
      Parsed [   0.112s] (baseline)
    Checking datafusion-proto-models v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   1.911s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field sort_order_for_reorder of struct ParquetScanExecNode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/fed3cde30a688259764e85f2b9e13262198ea752/datafusion/proto-models/src/generated/prost.rs:1974
  field reverse_row_groups of struct ParquetScanExecNode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/fed3cde30a688259764e85f2b9e13262198ea752/datafusion/proto-models/src/generated/prost.rs:1976
  field sort_order_for_reorder of struct ParquetScanExecNode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/fed3cde30a688259764e85f2b9e13262198ea752/datafusion/proto-models/src/generated/prost.rs:1974
  field reverse_row_groups of struct ParquetScanExecNode, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/fed3cde30a688259764e85f2b9e13262198ea752/datafusion/proto-models/src/generated/prost.rs:1976

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  44.048s] datafusion-proto-models
    Building datafusion-sqllogictest v55.0.0 (current)
       Built [  80.688s] (current)
     Parsing datafusion-sqllogictest v55.0.0 (current)
      Parsed [   0.019s] (current)
    Building datafusion-sqllogictest v55.0.0 (baseline)
       Built [  80.255s] (baseline)
     Parsing datafusion-sqllogictest v55.0.0 (baseline)
      Parsed [   0.020s] (baseline)
    Checking datafusion-sqllogictest v55.0.0 -> v55.0.0 (no change; assume patch)
     Checked [   0.100s] 223 checks: 223 pass, 31 skip
     Summary no semver update required
    Finished [ 163.645s] datafusion-sqllogictest

codecov-commenter · 2026-08-28T10:08:44Z

Codecov Report

❌ Patch coverage is 94.73684% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.63%. Comparing base (d2b626c) to head (ac49e2e).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/common/src/parquet_config.rs	85.71%	8 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #24642    +/-   ##
========================================
  Coverage   81.63%   81.63%            
========================================
  Files        1123     1123            
  Lines      409537   409674   +137     
  Branches   409537   409674   +137     
========================================
+ Hits       334308   334424   +116     
- Misses      55594    55610    +16     
- Partials    19635    19640     +5

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

kosiew · 2026-08-29T08:45:39Z

@zhangxinyao88
Can you resolve the CI error?

zhangxinyao88 · 2026-08-29T19:36:26Z

@zhangxinyao88 Can you resolve the CI error?

Yes - And thank you for reviewing my PR

kosiew

@zhangxinyao88,

Thanks for addressing the previous review comments. The feature-gated config test, failed SET mutation, and proto-models malformed decoder coverage all look addressed.

I found one remaining issue around RESET handling. Once that is fixed and covered by a regression test, I think this will be in good shape.

kosiew · 2026-09-01T09:22:49Z

+        Ok(())
+    }
+
+    fn reset(&mut self, _key: &str) -> Result<()> {


Could we reject nonempty keys here instead of ignoring _key? Right now, something like RESET datafusion.execution.parquet.statistics_enabled.typo is accepted and clears the entire statistics_enabled setting. The surrounding config namespace delegates the remaining suffix to this method, so a typo can silently mutate the configuration.

Please reject nonempty keys, consistent with the scalar ConfigField implementations, and add a regression test covering a nested RESET with an invalid suffix.

Fixed. A nested RESET now returns an error and leaves the current statistics setting unchanged. I added a regression test for the invalid suffix.

kosiew

@zhangxinyao88,

Thanks for the follow-up. The previous issues around feature gating, preserving the unset state after an invalid SET, proto-models validation, and nested RESET handling look addressed.

There is one remaining issue with nested config keys. The SET path still accepts a nonempty suffix, so I think this needs one more small fix before merging.

kosiew · 2026-09-02T03:20:49Z

+        }
+    }
+
+    fn set(&mut self, _key: &str, value: &str) -> Result<()> {


Thanks for fixing the nested RESET case. I think we need the same validation here for SET as well.

set currently ignores _key, so something like SET datafusion.execution.parquet.statistics_enabled.typo = 'none' is still accepted and updates statistics_enabled. A typo in a nested field should return an error rather than silently changing the scalar option.

Could you reject nonempty keys here, and in the scalar DFParquetStatistics implementation for consistency? It would also be good to add a regression assertion showing that a nested SET fails and leaves the existing value unchanged.

Fixed in cdc9deb. Nested SET now returns an error without changing the current value, with coverage for both option and scalar paths.

kosiew · 2026-09-04T03:36:54Z

@zhangxinyao88
Can you resolve the merge conflicts?

zhangxinyao88 · 2026-09-04T12:58:23Z

@kosiew Resolved the conflicts in cc890a7. Targeted tests, SET/RESET SQL tests, formatting, and Clippy passed.

The full suite stopped on a path-with-spaces issue that I also reproduced on clean main. That test passed after rebuilding in a path without spaces; the remaining suite hasn't run locally.

github-actions Bot added sqllogictest SQL Logic Tests (.slt) common Related to common crate proto Related to proto crate datasource Changes to the datasource crate labels Aug 24, 2026

zhangxinyao88 force-pushed the codex/datafusion-17498-statistics-validation branch from 7192caa to 99c04f2 Compare August 24, 2026 23:54

kosiew requested changes Aug 28, 2026

View reviewed changes

github-actions Bot added the auto detected api change Auto detected API change label Aug 28, 2026

zhangxinyao88 force-pushed the codex/datafusion-17498-statistics-validation branch from 99c04f2 to c4b56d3 Compare August 29, 2026 17:04

fix: validate parquet statistics config

ec51f9b

zhangxinyao88 force-pushed the codex/datafusion-17498-statistics-validation branch from c4b56d3 to ec51f9b Compare August 29, 2026 19:34

zhangxinyao88 requested a review from kosiew August 29, 2026 19:36

Merge branch 'main' into codex/datafusion-17498-statistics-validation

630e5f5

kosiew requested changes Sep 1, 2026

View reviewed changes

fix: reject nested parquet statistics reset

661f7fc

kosiew requested changes Sep 2, 2026

View reviewed changes

zhangxinyao88 added 2 commits September 1, 2026 23:40

fix: reject nested parquet statistics set

cdc9deb

Merge branch 'main' into codex/datafusion-17498-statistics-validation

ac49e2e

Merge upstream main and resolve Parquet config conflicts

cc890a7

Merge branch 'main' into codex/datafusion-17498-statistics-validation

f2cfb24

zhangxinyao88 requested a review from kosiew September 5, 2026 04:11

                       );
                   }
+                  #[test]

Uh oh!

Conversation

zhangxinyao88 commented Aug 24, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kosiew commented Aug 29, 2026

Uh oh!

zhangxinyao88 commented Aug 29, 2026

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kosiew commented Sep 4, 2026

Uh oh!

zhangxinyao88 commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Aug 28, 2026 •

edited

Loading

codecov-commenter commented Aug 28, 2026 •

edited

Loading