Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`. [datafusion]

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #12232:
URL: https://github.com/apache/datafusion/pull/12232#discussion_r1747754562


##########
datafusion/core/src/datasource/file_format/parquet.rs:
##########
@@ -227,6 +230,17 @@ impl ParquetFormat {
     pub fn options(&self) -> &TableParquetOptions {
         &self.options
     }
+
+    /// Return `true` if should use view types.
+    pub fn should_use_view_types(&self) -> bool {
+        self.options.global.schema_force_string_view
+    }
+
+    #[cfg(test)]
+    fn with_view_types(mut self, use_views: bool) -> Self {

Review Comment:
   I think this is a useful API and we should make it public for others to use who construct ListingTables programatically
   
   ```suggestion
       pub fn with_view_types(mut self, use_views: bool) -> Self {
   ```



##########
datafusion/core/src/datasource/file_format/parquet.rs:
##########
@@ -227,6 +230,17 @@ impl ParquetFormat {
     pub fn options(&self) -> &TableParquetOptions {
         &self.options
     }
+
+    /// Return `true` if should use view types.

Review Comment:
   This is a great idea (to put the flag on the format). I think it would also be a great place to document more of what this means. Something like
   
   
   ```suggestion
       /// Return `true` if should use view types.
       ///
       /// If this returns true, DataFusion will instruct the parquet reader
       /// to  read string / binary columns using view `StringView` or `BinaryView`
       ///  if the table schema specifies those types, regardless of any embedded metadata
       /// that may specify an alternate Arrow type. The parquet reader is optimized 
       /// for reading `StringView` and `BinaryView` and such queries are significantly faster. 
       ///
       ///  If this returns false, the parquet reader will read the columns according to the
       /// defaults or any embedded Arrow type information. This may result in reading 
       /// `StringArrays` and then casting to `StringViewArray` which is less efficient
   ```



##########
datafusion/common/src/config.rs:
##########
@@ -380,6 +380,10 @@ config_namespace! {
         /// the filters are applied in the same order as written in the query
         pub reorder_filters: bool, default = false
 
+        /// (reading) If true, parquet reader will read columns of `Utf8/Utf8Large` with `Utf8View`,

Review Comment:
   is this change so that all the reading configuration values are before the writing ones?



##########
datafusion/core/src/datasource/file_format/parquet.rs:
##########
@@ -227,6 +230,17 @@ impl ParquetFormat {
     pub fn options(&self) -> &TableParquetOptions {
         &self.options
     }
+
+    /// Return `true` if should use view types.

Review Comment:
   Also, what do you think about calling this `force_view_types` rather than `should_use_view_types`?
   
   Likewise then the setter would be named `with_force_view_types`
   
   



##########
datafusion/core/src/datasource/file_format/parquet.rs:
##########
@@ -1316,6 +1327,14 @@ mod tests {
         Ok(())
     }
 
+    #[tokio::test]
+    async fn read_merged_batches() -> Result<()> {

Review Comment:
   I suggest using a enum here (and in the other tests like this below) rather than a bool so it is easier to understand what is happening at the callsite
   
   For example: 
   ```rust
           _run_read_merged_batches(ForceViews::Yes).await?;
           _run_read_merged_batches(ForceViews::Yes).await?;
   ```



##########
datafusion/common/src/config.rs:
##########
@@ -380,6 +380,10 @@ config_namespace! {
         /// the filters are applied in the same order as written in the query
         pub reorder_filters: bool, default = false
 
+        /// (reading) If true, parquet reader will read columns of `Utf8/Utf8Large` with `Utf8View`,

Review Comment:
   While reviewing / considering this PR, I wonder if we should (in a follow on PR) rename this config flag to be `schema_force_view_types` as it also applies to binary columns 🤔 



##########
datafusion/core/src/datasource/file_format/mod.rs:
##########
@@ -229,6 +229,53 @@ pub fn transform_schema_to_view(schema: &Schema) -> Schema {
     Schema::new_with_metadata(transformed_fields, schema.metadata.clone())
 }
 
+/// Merge table schema into the parquet file schema.
+///
+/// This coerces the file schema if the table schema uses a view type.
+pub(crate) fn merge_file_schema_on_view_type(

Review Comment:
   Is this function  isn't really "merging" the schemas in the sense that it would add missing fields. Instead it is more like coercing the file schema to natively read string / binary columns using view types if the table schema specifies those types - otherwise the parquet reader would read the columns as StringArrays and then cast to StringViewArray
   
    Maybe we can rename this function to `merge_file_schema_on_view_type` to better reflect this



-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org