kosiew commented on code in PR #24642:
URL: https://github.com/apache/datafusion/pull/24642#discussion_r3902677733
##########
datafusion/common/src/parquet_config.rs:
##########
@@ -106,3 +106,97 @@ impl From<parquet::file::properties::WriterVersion> for
DFParquetWriterVersion {
}
}
}
+
+/// Parquet statistics levels supported by the writer
+///
+/// This enum validates statistics settings at configuration time, ensuring
only
+/// `none`, `chunk`, or `page` can be set via `SET` commands or
deserialization.
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub enum DFParquetStatistics {
+ /// Do not write statistics
+ None,
+ /// Write chunk-level statistics
+ Chunk,
+ /// Write page-level statistics
+ Page,
+}
+
+impl FromStr for DFParquetStatistics {
+ type Err = DataFusionError;
+
+ fn from_str(s: &str) -> Result<Self, Self::Err> {
+ match s.to_lowercase().as_str() {
+ "none" => Ok(Self::None),
+ "chunk" => Ok(Self::Chunk),
+ "page" => Ok(Self::Page),
+ other => Err(DataFusionError::Configuration(format!(
+ "Invalid parquet statistics setting: {other}. Expected one of:
none, chunk, page"
+ ))),
+ }
+ }
+}
+
+impl Display for DFParquetStatistics {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ let s = match self {
+ Self::None => "none",
+ Self::Chunk => "chunk",
+ Self::Page => "page",
+ };
+ f.write_str(s)
+ }
+}
+
+impl ConfigField for DFParquetStatistics {
+ fn visit<V: Visit>(&self, v: &mut V, key: &str, description: &'static str)
{
+ v.some(key, self, description)
+ }
+
+ fn set(&mut self, _: &str, value: &str) -> Result<()> {
+ *self = Self::from_str(value)?;
+ Ok(())
+ }
+}
+
+/// `ConfigField` for `Option<DFParquetStatistics>` parses before assigning so
+/// an invalid value does not turn an unset option into the default.
+impl ConfigField for Option<DFParquetStatistics> {
+ fn visit<V: Visit>(&self, v: &mut V, key: &str, description: &'static str)
{
+ match self {
+ Some(statistics) => statistics.visit(v, key, description),
+ None => v.none(key, description),
+ }
+ }
+
+ fn set(&mut self, _key: &str, value: &str) -> Result<()> {
+ *self = Some(DFParquetStatistics::from_str(value)?);
+ Ok(())
+ }
+
+ fn reset(&mut self, _key: &str) -> Result<()> {
Review Comment:
Could we reject nonempty keys here instead of ignoring `_key`? Right now,
something like `RESET datafusion.execution.parquet.statistics_enabled.typo` is
accepted and clears the entire `statistics_enabled` setting. The surrounding
config namespace delegates the remaining suffix to this method, so a typo can
silently mutate the configuration.
Please reject nonempty keys, consistent with the scalar `ConfigField`
implementations, and add a regression test covering a nested `RESET` with an
invalid suffix.
--
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]
