You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "wiedld (via GitHub)" <gi...@apache.org> on 2024/08/29 10:38:22 UTC
[PR] Parquet statistic on read with `schema_force_string_view`. [datafusion]
wiedld opened a new pull request, #12232:
URL: https://github.com/apache/datafusion/pull/12232
## Which issue does this PR close?
Closes #12123
## Rationale for this change
On write: parquet file written with utf8/large-utf & binary/large-binary schema (so is in metadata).
On read: we would like to be able to read as the more performant view types.
Previous work has already used the `schema_force_string_view` to read into utf8view and binaryview, by [passing around a bool to the ParquetOpener](https://github.com/apache/datafusion/blob/bd506980bd04c109d9fa979be5b627580e59d267/datafusion/core/src/datasource/physical_plan/parquet/opener.rs#L104-L106).
This work is to focus on getting the parquet statistics, on read, to properly compute when reading as view types.
## What changes are included in this PR?
* move the `schema_force_string_view` up a few lines, to be with the "read" (not write) config options.
* remove the passing around of bools
* this was done by merging table_schema (with views) and file_schema (without views)
* add tests which run with true|false for `schema_force_string_view`
**WIP, still debugging:** there is an earlier draft PR [which has all CI ](https://github.com/apache/datafusion/pull/11862/files#r1727710645) tests passing when the `schema_force_string_view=true`. However, I'm not quite sure how that worked since the [page level statistics](https://github.com/apache/arrow-rs/blob/master/parquet/src/arrow/arrow_reader/statistics.rs#L796) are not implemented for view types. I made a test to capture this gap.
Additionally, we still have statistics failing -- but only for one test case (not others using view types). I'll chase down tmrw what's different in that test.
## Are these changes tested?
Yes.
## Are there any user-facing changes?
No.
--
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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`, rename config option to `schema_force_view_types` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #12232:
URL: https://github.com/apache/datafusion/pull/12232#discussion_r1750569112
##########
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:
Thank you -- this is much easier to follow now
--
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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12232:
URL: https://github.com/apache/datafusion/pull/12232#discussion_r1750504243
##########
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:
renamed and docs added.
--
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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`, rename config option to `schema_force_view_types` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12232:
URL: https://github.com/apache/datafusion/pull/12232#issuecomment-2340410863
Thanks again @wiedld
--
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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12232:
URL: https://github.com/apache/datafusion/pull/12232#discussion_r1750505737
##########
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:
Ah...I just did it all in this PR because it felt weird to have 2 naming conventions at once. Hopefully that's ok. 😅
--
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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12232:
URL: https://github.com/apache/datafusion/pull/12232#discussion_r1750547701
##########
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:
Nice. Done!
--
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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`, rename config option to `schema_force_view_types` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #12232:
URL: https://github.com/apache/datafusion/pull/12232
--
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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`. [datafusion]
Posted by "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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12232:
URL: https://github.com/apache/datafusion/pull/12232#discussion_r1750497728
##########
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:
Renamed to `coerce_file_schema_to_view_type`.
--
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
Re: [PR] Fix parquet statistics for ListingTable and Utf8View with `schema_force_string_view`. [datafusion]
Posted by "wiedld (via GitHub)" <gi...@apache.org>.
wiedld commented on code in PR #12232:
URL: https://github.com/apache/datafusion/pull/12232#discussion_r1750505737
##########
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:
Ah...I just did it all in this PR. Hopefully that's ok. 😅
--
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