Skip to content

Parquet: Add tests for disabling statistics on multiple columns - #18096

Open
LouisDeconinck wants to merge 2 commits into
apache:mainfrom
LouisDeconinck:fix-15347-disable-metrics-multi-column
Open

LouisDeconinck wants to merge 2 commits into
apache:mainfrom
LouisDeconinck:fix-15347-disable-metrics-multi-column

Conversation

@LouisDeconinck

Copy link
Copy Markdown

Summary

Adds test coverage in TestParquet for disabling Parquet column statistics via write.parquet.stats-enabled.column.<COLUMN_NAME> on multiple columns at once, the scenario reported in #15347.

The multi-column disable path already works correctly on main (as confirmed in the issue discussion and in the earlier attempts), so this PR adds regression coverage only — no production code changes.

A shared writeAndAssertColumnStatistics helper writes records with the given table properties and asserts per-column statistics presence in the Parquet footer. All of the tests share one constant schema and express expectations as a column name to statistics-presence map, per the review feedback on #17365:

  • testColumnStatisticsDefault — no per-column properties; all columns write statistics
  • testColumnStatisticsEnabled — existing test, refactored onto the shared schema and helper (one column enabled, one disabled, one unconfigured)
  • testMultipleColumnsStatisticsDisabled — two columns disabled; both omit statistics while the unconfigured third column still writes them (the reported Disabling statistics across multiple columns #15347 scenario)
  • testAllColumnsStatisticsDisabled — statistics disabled on every column in the schema

Fixes #15347

Test plan

  • ./gradlew :iceberg-parquet:test --tests 'org.apache.iceberg.parquet.TestParquet' — 26 tests, all pass
  • ./gradlew :iceberg-parquet:spotlessCheck — clean

AI Disclosure

@yangshangqing95 yangshangqing95 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.
Nit, non-blocking: could we remove the test prefix from the newly added test method names?

@LouisDeconinck

Copy link
Copy Markdown
Author

Done — the three new test methods now drop the test prefix (columnStatisticsDefault, multipleColumnsStatisticsDisabled, allColumnsStatisticsDisabled), matching the project convention. The pre-existing testColumnStatisticsEnabled keeps its name.

@LouisDeconinck

Copy link
Copy Markdown
Author

Done — dropped the test prefix from the newly added test methods (columnStatisticsDefault, multipleColumnsStatisticsDisabled, allColumnsStatisticsDisabled).

@LouisDeconinck

Copy link
Copy Markdown
Author

Thanks! Dropped the test prefix from the new test method names (columnStatisticsDefault, multipleColumnsStatisticsDisabled, allColumnsStatisticsDisabled).

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Disabling statistics across multiple columns

2 participants