kosiew commented on code in PR #24642:
URL: https://github.com/apache/datafusion/pull/24642#discussion_r3910408685
##########
datafusion/common/src/parquet_config.rs:
##########
@@ -106,3 +106,104 @@ 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<()> {
Review Comment:
Thanks for fixing the nested `RESET` case. I think we need the same
validation here for `SET` as well.
`set` currently ignores `_key`, so something like `SET
datafusion.execution.parquet.statistics_enabled.typo = 'none'` is still
accepted and updates `statistics_enabled`. A typo in a nested field should
return an error rather than silently changing the scalar option.
Could you reject nonempty keys here, and in the scalar `DFParquetStatistics`
implementation for consistency? It would also be good to add a regression
assertion showing that a nested `SET` fails and leaves the existing value
unchanged.
--
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]
