sunchao commented on code in PR #5602:
URL: https://github.com/apache/datafusion-comet/pull/5602#discussion_r3899970855
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -338,7 +354,7 @@ fn check_column_duplicate(col_name: &str, physical_schema:
&SchemaRef) -> Option
let matches: Vec<&str> = physical_schema
.fields()
.iter()
- .filter(|pf| pf.name().eq_ignore_ascii_case(col_name))
+ .filter(|pf| names_match(pf.name(), col_name, false))
Review Comment:
[P2] Preserve field-ID precedence in the Unicode duplicate check
The Unicode-name coverage is useful. Could we retain ID-based resolution
when applying this duplicate check? With
`spark.sql.parquet.fieldId.read.enabled=true` and
`spark.sql.caseSensitive=false`, an explicit one-field schema `ω` (ID 2) can
select a file's `ω` (ID 2) even when the file also contains `Ω` (ID 1). A
case-sensitive writer with field-ID writing enabled can produce these two
nullable integer columns with varying values. The ID remap selects ID 2, but
this check examines the original physical schema and now counts both names,
returning `DuplicateFieldCaseInsensitive` before the valid projection. Spark
selects the unique ID before considering names, and the previous ASCII check
admits this Unicode example. Please retain ambiguity errors for name-based
reads and add a regression case for the ID-based read. This example is
source-derived, not an executed reproduction.
##########
native/core/src/parquet/schema_adapter.rs:
##########
@@ -76,6 +76,22 @@ fn schema_has_field_ids(schema: &SchemaRef) -> bool {
schema.fields().iter().any(|f| parse_field_id(f).is_some())
}
+/// Compare two field names under Spark's Parquet case-folding rules. In
+/// case-sensitive mode the names must be identical. In case-insensitive mode
+/// this mirrors `ParquetReadSupport`, which resolves fields by grouping on
+/// `name.toLowerCase(Locale.ROOT)`. `str::to_lowercase` is the Unicode-aware
+/// equivalent of Java's `toLowerCase(Locale.ROOT)`, so non-ASCII case
+/// differences (e.g. `Ä`/`ä`, `Ω`/`ω`) fold the same way Spark folds them.
+/// ASCII-only folding (`eq_ignore_ascii_case`) would leave those unmatched and
+/// silently read the column as nulls.
+fn names_match(a: &str, b: &str, case_sensitive: bool) -> bool {
+ if case_sensitive {
+ a == b
+ } else {
+ a.to_lowercase() == b.to_lowercase()
Review Comment:
[P2] Match Java's contextual sigma lowercasing
There is a contextual difference in addition to the Unicode-table version
differences already discussed. In the inspected Rust 1.88.0 and OpenJDK 17u
17.0.16+8 default-provider sources, `A1Σ` lowercases to `a1σ` in Rust but `a1ς`
in Java's `Locale.ROOT` path. Java searches across the digit within a word,
whereas Rust stops its cased-character search at that digit. With field-ID
matching disabled, an explicit nullable `A1σ` Parquet projection therefore
binds the stored values through this comparison where Spark and the previous
ASCII comparison supply nulls. Could we preserve the JVM Parquet
name-resolution contract for this context and cover it in a regression test?
This is source-derived, not a runtime reproduction or a claim about every
JDK/compiler version.
--
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]
