kumarUjjawal commented on code in PR #24227:
URL: https://github.com/apache/datafusion/pull/24227#discussion_r3911164931
##########
datafusion/datasource-parquet/src/metadata.rs:
##########
@@ -439,6 +448,68 @@ impl<'a> DFParquetMetadata<'a> {
.coerce()
})
.unwrap_or(schema);
+
+ let schema = if self.enable_rle_to_dictionary {
+ let schema_descr = file_metadata.schema_descr();
+ // Top-level columns that have a dictionary page in at least one
row group.
+ let dict_cols: HashSet<String> = metadata
+ .row_groups()
+ .iter()
+ .flat_map(|rg| {
+ rg.columns()
+ .iter()
+ .enumerate()
+ .filter_map(|(col_idx, col)| {
+ col.dictionary_page_offset()?;
Review Comment:
These hosted benchmarks do not appear to exercise this feature:
enable_rle_to_dictionary defaults to false, and the reported run configuration
does not enable it. They also target 35dd8d8, not the current head. Since a
dictionary-page offset does not guarantee that later pages remain dictionary
encoded, could we add current-head enabled/disabled comparisons for both
low-cardinality data and a high-cardinality dictionary-fallback column,
including peak memory?
##########
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(
+ field_type: &DataType,
+ dictionary_value_type: &DataType,
+) -> Option<DataType> {
+ let field_type = match field_type {
+ DataType::Dictionary(_, field_value_type) => field_value_type.as_ref(),
+ _ => field_type,
+ };
+
+ match (field_type, dictionary_value_type) {
+ (DataType::Utf8, DataType::Utf8) => Some(DataType::Utf8),
+ (DataType::Utf8 | DataType::LargeUtf8, DataType::Utf8 |
DataType::LargeUtf8) => {
+ Some(DataType::LargeUtf8)
+ }
+ (DataType::Binary, DataType::Binary) => Some(DataType::Binary),
+ (
+ DataType::Binary | DataType::LargeBinary,
+ DataType::Binary | DataType::LargeBinary,
+ ) => Some(DataType::LargeBinary),
+ _ => None,
+ }
+}
+
+/// Allows safe widening into the table dictionary type.
+/// - `Utf8` to `Dictionary(Int32, LargeUtf8)`: allowed
+/// - `LargeBinary` to `Dictionary(Int32, Binary)`: rejected
+fn can_promote_to_dictionary_type(
+ file_field_type: &DataType,
+ table_dictionary_type: &DataType,
+) -> bool {
+
dictionary_value_type(table_dictionary_type).is_some_and(|dictionary_value_type|
{
+ common_dictionary_value_type(file_field_type, dictionary_value_type)
+ .is_some_and(|common_type| &common_type == dictionary_value_type)
+ })
+}
+
+/// Normalize per-file schemas so that a column promoted to `Dictionary` in
+/// *any* file is promoted to the same `Dictionary` type in *all* files.
+///
+/// This lets [`Schema::try_merge`] accept directories that mix dictionary and
+/// plain encodings for the same column.
+pub(crate) fn uniform_dict_schemas(schemas: Vec<Schema>) -> Vec<Schema> {
+ // First pass: record the dictionary type for every column that is
Dictionary in
+ // at least one schema.
+ let mut dict_types: HashMap<String, DataType> = HashMap::new();
+ for schema in &schemas {
+ for field in schema.fields() {
+ if matches!(field.data_type(), DataType::Dictionary(_, _)) {
+ dict_types
+ .entry(field.name().clone())
+ .or_insert_with(|| field.data_type().clone());
+ }
+ }
+ }
+ if dict_types.is_empty() {
+ return schemas;
+ }
+
+ // Widen the recorded dictionary value type before promoting plain fields.
+ for schema in &schemas {
+ for field in schema.fields() {
+ let Some(dict_type) = dict_types.get_mut(field.name()) else {
+ continue;
+ };
+ let DataType::Dictionary(key_type, value_type) = dict_type else {
Review Comment:
dict_types preserves the key type from the first dictionary schema, but
can_promote_to_dictionary_type compares only value types. A mixed
Dictionary(Int8, Utf8) / Dictionary(Int32, Utf8) directory can therefore be
normalized to Int8, and Arrow will fail once a later dictionary has more than
127 values. Could this choose a safe common key type at least Int32 for
feature-promoted columns and add a mixed-key/high-cardinality regression test?
--
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]
