kosiew commented on code in PR #24642:
URL: https://github.com/apache/datafusion/pull/24642#discussion_r3879310809
##########
datafusion/common/src/config.rs:
##########
@@ -1419,7 +1419,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)
Review Comment:
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`.
##########
datafusion/proto-models/src/from_proto.rs:
##########
@@ -366,13 +366,15 @@ impl TryFrom<&ParquetOptionsProto> for ParquetOptions {
}
}),
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
Review Comment:
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.
##########
datafusion/common/src/config.rs:
##########
@@ -4577,6 +4577,32 @@ mod tests {
);
}
+ #[test]
Review Comment:
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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
