zhangxinyao88 commented on code in PR #24642:
URL: https://github.com/apache/datafusion/pull/24642#discussion_r3887467725
##########
datafusion/common/src/config.rs:
##########
@@ -4577,6 +4577,32 @@ mod tests {
);
}
+ #[test]
Review Comment:
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.
##########
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:
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.
##########
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:
Added a negative decoder test for an invalid parquet statistics value as
well.
--
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]
