kumarUjjawal commented on code in PR #24227:
URL: https://github.com/apache/datafusion/pull/24227#discussion_r3878078785
##########
datafusion/datasource-parquet/src/schema_coercion.rs:
##########
@@ -438,6 +568,25 @@ pub fn transform_schema_to_view(schema: &Schema) -> Schema
{
DataType::Binary | DataType::LargeBinary => {
field_with_new_type(field, DataType::BinaryView)
}
+ // Also rewrite the value type inside dictionary columns so that
+ // dict<_, Utf8> / dict<_, LargeUtf8> become dict<_, Utf8View> and
+ // dict<_, Binary> / dict<_, LargeBinary> become dict<_,
BinaryView>.
+ DataType::Dictionary(key_type, value_type) => {
Review Comment:
This arm runs whenever `schema_force_view_types` is on, which is the
default, and it does not look at `enable_rle_to_dictionary`. With the flag off,
`DESCRIBE dict_hash_10` in
`dictionary.slt` returns `Dictionary(Int32, Utf8View)` instead of
`Dictionary(Int32, Utf8)`. The same change makes
`test_statistics_from_parquet_metadata_dictionary` return `Absent` for
`null_count`, so the column loses its statistics and its row group
pruning. `transform_binary_to_string` gained the same unconditional arm. Would
it work to drop both arms and keep the
view rewrite out of this PR?
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -402,7 +426,12 @@ impl FileFormat for ParquetFormat {
}
drop(seen);
- let schemas = schemas.into_iter().map(|(_, schema)| schema);
+ // Normalize dict-promoted schemas before merging so mixed dict/plain
files merge cleanly.
+ let mut schemas: Vec<Schema> =
+ schemas.into_iter().map(|(_, schema)| schema).collect();
Review Comment:
This gate reads only the global flag, but `fetch_schema` promotes when
either the flag or the allowlist is set. So an allowlist with the flag off
promotes per file and then skips this normalization. A directory that mixes
dictionary and plain files then fails `Schema::try_merge` at registration.
Would it work to use the same condition here that `fetch_schema` uses?
##########
datafusion/datasource-parquet/src/file_format.rs:
##########
@@ -210,6 +213,22 @@ impl ParquetFormat {
&self.options
}
+ /// Restrict RLE->Dictionary promotion to a named subset of columns.
+ ///
+ /// Only columns in `columns` that also have dictionary pages in the file
+ /// are promoted to `Dictionary(Int32, …)` in the inferred schema.
+ /// Overrides `enable_rle_to_dictionary` when set; an empty set disables
+ /// promotion entirely. See
<https://github.com/apache/datafusion/issues/24113>.
+ pub fn with_rle_column_allowlist(mut self, columns: HashSet<String>) ->
Self {
Review Comment:
Nothing in the tree calls `with_rle_column_allowlist` or
`rle_column_allowlist`, and no test covers the allowlist path. New public API
on `ParquetFormat` is hard to remove later, and the shape of #24113 can still
change. I'd leave the allowlist out of this PR and add it with the per-column
work that needs it.
##########
datafusion/datasource-parquet/src/schema_coercion.rs:
##########
@@ -135,6 +157,114 @@ pub fn apply_file_schema_type_coercions(
))
}
+fn dictionary_value_type(data_type: &DataType) -> Option<&DataType> {
+ match data_type {
+ DataType::Dictionary(_, value_type) => Some(value_type.as_ref()),
+ _ => None,
+ }
+}
+
+// Find the value type that can represent both sides without narrowing offsets
+// or crossing string/binary families.
+fn common_dictionary_value_type(
Review Comment:
This helper has no arm for `Utf8View` or `BinaryView`, so
`can_promote_to_dictionary_type` returns false for every view value type. With
`schema_force_view_types` on, inference produces
`Dictionary(Int32, Utf8View)`, and the opener then declines to coerce the
file column. Am I right that the dictionary array then comes from a later cast,
not from arrow-rs?
--
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]
