Parquet: Fix per-column statistics disabling across multiple columns - #17461
Parquet: Fix per-column statistics disabling across multiple columns#17461Alwaysgaurav1 wants to merge 2 commits into
Conversation
Closes apache#15347. setColumnStatsConfig in Parquet.java previously applied per-column statistics enablement via withColumnStatsEnabled.accept(...) but did not set parquet.column.statistics.enabled#<column> in the Hadoop Configuration instance. When configuring multiple columns to false via write.parquet.stats-enabled.column.<column>, only the first column's statistics were disabled. This change explicitly passes per-column statistics enablement settings into conf as well, ensuring consistent behavior across multiple columns. Added unit test testMultipleColumnStatisticsDisabled in TestParquet.java.
| boolean enabled = Boolean.parseBoolean(isEnabled); | ||
| withColumnStatsEnabled.accept(parquetColumnPath, enabled); | ||
| conf.set( | ||
| "parquet.column.statistics.enabled#" + parquetColumnPath, |
There was a problem hiding this comment.
The added conf.set("parquet.column.statistics.enabled#" + parquetColumnPath, String.valueOf(enabled)) is dead code. The key parquet.column.statistics.enabled# is consumed only by ParquetOutputFormat.ColumnConfigParser (the Hadoop MapReduce write path); Iceberg never invokes ParquetOutputFormat, so this key is never read back by either the createWriterFunc path (ParquetProperties.Builder, line 487) or the ParquetWriteBuilder path (line 532). The real per-column setting routes through withColumnStatsEnabled.accept(parquetColumnPath, enabled) at line 367 → propsBuilder::withStatisticsEnabled / parquetWriteBuilder::withStatisticsEnabled → ColumnProperty.Builder HashMap, which already handles multiple columns independently. The production change is based on a false root-cause diagnosis and should be reverted entirely.
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
No production fix is needed: the multi-column statistics disable path already works on current main. PR #17365 (open, approved) adds equivalent test coverage without any production change and CI is green, confirming the multi-column path is not broken. This PR's test testMultipleColumnStatisticsDisabled does not guard a real regression; it passes with or without the conf.set because the actual enforcement runs through ParquetProperties.Builder, not conf.
2 participants
Closes #15347.
Parquet.setColumnStatsConfigpreviously applied per-column statistics enablement viawithColumnStatsEnabled.accept(...)onParquetProperties.Builder/ParquetWriter.Builderbut did not propagateparquet.column.statistics.enabled#<column>into the HadoopConfigurationinstance (conf).When users configured multiple table properties to disable statistics across multiple columns (e.g.,
write.parquet.stats-enabled.column.colA = falseandwrite.parquet.stats-enabled.column.colB = false), statistics were only disabled for the first column processed while subsequent columns continued to write statistics into the Parquet file header.This change explicitly passes per-column statistics enablement settings into
confas well (parquet.column.statistics.enabled#<parquetColumnPath>), ensuring consistent behavior across all configured columns regardless of write path.Tests Added
testMultipleColumnStatisticsDisabledinTestParquet.java: Validates that configuringwrite.parquet.stats-enabled.column.<col> = falseacross multiple columns disables statistics for all specified columns in the generated Parquet file.