Copilot commented on code in PR #12726: URL: https://github.com/apache/gluten/pull/12726#discussion_r4136919675
########## docs/velox-backend-limitations.md: ########## @@ -21,7 +21,65 @@ Gluten currently doesn't support ANSI mode. If ANSI is enabled, Spark plan's exe We now have a issue tracker on ANSI support progress. Please check [issue-10134](https://github.com/apache/gluten/issues/10134). #### Case Sensitive mode -Gluten only supports spark default case-insensitive mode. If case-sensitive mode is enabled, user may get incorrect result. +Gluten respects Spark's case-sensitive configuration (`spark.sql.caseSensitive`). Since +[GLUTEN-1577](https://github.com/apache/gluten/issues/1577) (merged 2023-05), column-name +normalisation in the core engine uses `ConverterUtils.normalizeColName`, which preserves the +original casing when `caseSensitiveAnalysis=true` and lowercases only when it is `false` (the +Spark default). Standard data operations such as scan, filter, aggregation, and join are +therefore correct in both modes. Review Comment: This broad statement is contradicted by the same section's known pre-existing issue: mixed-case Iceberg `GROUP BY` can still return incorrect native results at lines 76-82. Please qualify the claim to core paths covered by this change rather than stating that scan/filter/aggregation/join are correct in both modes. ########## docs/velox-backend-limitations.md: ########## @@ -21,7 +21,65 @@ Gluten currently doesn't support ANSI mode. If ANSI is enabled, Spark plan's exe We now have a issue tracker on ANSI support progress. Please check [issue-10134](https://github.com/apache/gluten/issues/10134). #### Case Sensitive mode -Gluten only supports spark default case-insensitive mode. If case-sensitive mode is enabled, user may get incorrect result. +Gluten respects Spark's case-sensitive configuration (`spark.sql.caseSensitive`). Since +[GLUTEN-1577](https://github.com/apache/gluten/issues/1577) (merged 2023-05), column-name +normalisation in the core engine uses `ConverterUtils.normalizeColName`, which preserves the +original casing when `caseSensitiveAnalysis=true` and lowercases only when it is `false` (the +Spark default). Standard data operations such as scan, filter, aggregation, and join are +therefore correct in both modes. + +**This change addresses the following identified metadata-name collision paths:** + +- `IcebergScanTransformer`: previously used unconditional `equalsIgnoreCase` in + `getMetadataColumns` and unconditional `toLowerCase` in the read-schema field set, causing a + user data column named `Input_File_Name` (or any mixed-case variant of an Iceberg metadata + column name) to be misclassified as a metadata column under `caseSensitive=true`. + Fixed by switching to `ConverterUtils.normalizeColName` throughout the Iceberg scan path. + Validated by `IcebergSuite` / `VeloxIcebergSuite`. + +- `PushDownInputFileExpression` (core rule, `gluten-substrait`): two unconditional + `toLowerCase` usages — one in `containsInputFileRelatedExpr` and one in the `PostOffload` + deduplication — caused incorrect pre-offload rewriting and dangling-attribute plan errors when + a data column named `Input_File_Name` was projected alongside `input_file_name()` under + `caseSensitive=true`. Fixed by using `ConverterUtils.normalizeColName` for gate detection and + `exprId` identity for deduplication. + Validated by `FallbackSuite` (Velox) and `IcebergSuite`. + +- **Delta optimised writer** (`GlutenDeltaOptimizedWriterExec` / `DeltaOptimizedWriterTransformer`): + previously used `caseInsensitiveResolution` (a hardcoded case-insensitive comparator) for + partition-column lookup, ignoring `spark.sql.caseSensitive=true`. Fixed by switching to + `SQLConf.get.resolver`, which honours the session case-sensitivity setting. + Full end-to-end Delta writer tests require a native Delta backend and are not run in CI for + this module; the resolver-semantics contract is validated at the unit level by + `GlutenClickHouseCaseSensitiveSchemaSuite`. Review Comment: This documentation claims that `GlutenClickHouseCaseSensitiveSchemaSuite` validates the Delta resolver change, but no such suite exists in the repository and this PR does not add it. Please either reference the actual test or state that this path is not covered, so the limitations document does not advertise nonexistent coverage. This issue also appears on line 56 of the same file. ########## gluten-substrait/src/main/scala/org/apache/gluten/execution/BasicScanExecTransformer.scala: ########## @@ -123,7 +123,9 @@ trait BasicScanExecTransformer extends LeafTransformSupport with BaseDataSource InputFileBlockLength().prettyName) val neededInputFileRelatedMetadataKeys = - inputFileRelatedMetadataKeys.filter(k => output.exists(_.name == k)) + inputFileRelatedMetadataKeys.filter { + k => output.exists(a => ConverterUtils.normalizeColName(a.name) == k) + } Review Comment: Under case-insensitive analysis this now treats an ordinary user column such as `Input_File_Name` as the `input_file_name` metadata column. For a query that selects only that data column, `replacedExprs` is empty, so the new conflict check does not tag the scan, but `metadataColumnNames` still requests the metadata handle and the native reader can return the file path instead of the user's value. This lookup needs to identify injected metadata attributes rather than normalizing every scan output name; please retain exact alias matching here and add coverage for selecting the mixed-case data column without `input_file_name()`. -- 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]
