-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: validate parquet statistics config #24642
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
base: main
Are you sure you want to change the base?
Changes from all commits
ec51f9b
630e5f5
661f7fc
cdc9deb
ac49e2e
cc890a7
f2cfb24
e20dd27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ use arrow_ipc::CompressionType; | |
| use crate::encryption::{FileDecryptionProperties, FileEncryptionProperties}; | ||
| use crate::error::{_config_datafusion_err, _config_err}; | ||
| use crate::format::{ExplainAnalyzeCategories, ExplainFormat, MetricType}; | ||
| use crate::parquet_config::DFParquetWriterVersion; | ||
| use crate::parquet_config::{DFParquetStatistics, DFParquetWriterVersion}; | ||
| use crate::parsers::{CompressionTypeVariant, CsvQuoteStyle}; | ||
| use crate::utils::get_available_parallelism; | ||
| use crate::{DataFusionError, Result}; | ||
|
|
@@ -1442,7 +1442,7 @@ config_namespace! { | |
| /// Valid values are: "none", "chunk", and "page" | ||
| /// 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) | ||
|
|
||
| /// (writing) Target maximum number of rows in each row group (defaults to 1M | ||
| /// rows). Writing larger row groups requires more memory to write, but | ||
|
|
@@ -4599,6 +4599,73 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[cfg(feature = "parquet")] | ||
| #[test] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you gate this test with
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed, thanks. I gated the parquet validation test on the |
||
| fn test_parquet_statistics_validation() { | ||
| use crate::{config::ConfigOptions, parquet_config::DFParquetStatistics}; | ||
|
|
||
| let mut config = ConfigOptions::default(); | ||
|
|
||
| for (value, expected) in [ | ||
| ("none", DFParquetStatistics::None), | ||
| ("CHUNK", DFParquetStatistics::Chunk), | ||
| ("page", DFParquetStatistics::Page), | ||
| ] { | ||
| config | ||
| .set("datafusion.execution.parquet.statistics_enabled", value) | ||
| .unwrap(); | ||
| assert_eq!(config.execution.parquet.statistics_enabled, Some(expected)); | ||
| } | ||
|
|
||
| let err = config | ||
| .set("datafusion.execution.parquet.statistics_enabled", "invalid") | ||
| .unwrap_err(); | ||
| assert_contains!( | ||
| err.to_string(), | ||
| "Invalid parquet statistics setting: invalid. Expected one of: none, chunk, page" | ||
| ); | ||
|
|
||
| // An unset value can arise from deserialization. An invalid update must | ||
| // leave that state unchanged rather than inserting the default. | ||
| config.execution.parquet.statistics_enabled = None; | ||
| assert_eq!(config.execution.parquet.statistics_enabled, None); | ||
|
|
||
| assert!( | ||
| config | ||
| .set("datafusion.execution.parquet.statistics_enabled", "invalid") | ||
| .is_err() | ||
| ); | ||
| assert_eq!(config.execution.parquet.statistics_enabled, None); | ||
|
|
||
| config.execution.parquet.statistics_enabled = Some(DFParquetStatistics::Page); | ||
| assert!( | ||
| config | ||
| .set( | ||
| "datafusion.execution.parquet.statistics_enabled.typo", | ||
| "none" | ||
| ) | ||
| .is_err() | ||
| ); | ||
| assert_eq!( | ||
| config.execution.parquet.statistics_enabled, | ||
| Some(DFParquetStatistics::Page) | ||
| ); | ||
|
|
||
| assert!( | ||
| config | ||
| .reset("datafusion.execution.parquet.statistics_enabled.typo") | ||
| .is_err() | ||
| ); | ||
| assert_eq!( | ||
| config.execution.parquet.statistics_enabled, | ||
| Some(DFParquetStatistics::Page) | ||
| ); | ||
|
|
||
| let mut scalar = DFParquetStatistics::Page; | ||
| assert!(ConfigField::set(&mut scalar, "typo", "none").is_err()); | ||
| assert_eq!(scalar, DFParquetStatistics::Page); | ||
| } | ||
|
|
||
| #[cfg(feature = "parquet")] | ||
| #[test] | ||
| fn set_cdc_enabled_flag() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -396,13 +396,15 @@ impl TryFrom<&ParquetOptionsProto> for ParquetOptions { | |
| proto.dictionary_page_size_limit, | ||
| "dictionary_page_size_limit", | ||
| )?, | ||
| statistics_enabled: proto.statistics_enabled_opt.as_ref().map( | ||
| |opt| match opt { | ||
| statistics_enabled: proto | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a negative decoder test for an invalid parquet statistics value as well. |
||
| .statistics_enabled_opt | ||
| .as_ref() | ||
| .map(|opt| match opt { | ||
| parquet_options::StatisticsEnabledOpt::StatisticsEnabled( | ||
| statistics, | ||
| ) => statistics.clone(), | ||
| }, | ||
| ), | ||
| ) => statistics.parse(), | ||
| }) | ||
| .transpose()?, | ||
| max_row_group_size: to_usize(proto.max_row_group_size, "max_row_group_size")?, | ||
| max_in_list_size: to_usize(proto.max_in_list_size, "max_in_list_size")?, | ||
| created_by: proto.created_by.clone(), | ||
|
|
@@ -561,3 +563,26 @@ impl TryFrom<&TableParquetOptionsProto> for TableParquetOptions { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn rejects_invalid_parquet_statistics() { | ||
| let proto = ParquetOptionsProto { | ||
| statistics_enabled_opt: Some( | ||
| parquet_options::StatisticsEnabledOpt::StatisticsEnabled( | ||
| "invalid".to_string(), | ||
| ), | ||
| ), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let err = ParquetOptions::try_from(&proto).unwrap_err(); | ||
| assert!( | ||
| err.to_string() | ||
| .contains("Invalid parquet statistics setting: invalid") | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a subtle state mutation here when this option is
None. The blanketOption<F>::setinsertsDFParquetStatistics::default()before trying to parse the new value. This means that afterRESET datafusion.execution.parquet.statistics_enabled, runningSET ... = 'invalid'correctly returns an error, but also changes the setting fromNonetoSome(Page).Could we use the parse-then-assign pattern used by
Option<MaxRowGroupBytes>so a failedSETleaves the existing configuration unchanged? It would also be good to add a regression assertion coveringRESETfollowed by an invalidSET.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks. The option now parses before assigning, so an invalid
SETleaves an unset value untouched.RESETrestores the configured default,Page, so the regression test explicitly starts from an unset value before trying the invalid update.