sabbasani commented on PR #12726: URL: https://github.com/apache/gluten/pull/12726#issuecomment-6358262514
> Some initial comments > > 1. Test Redundancy & Scope > The test additions contain significant duplication and out-of-scope test cases: > > Duplicate tests in FallbackSuite: Multiple test cases test the exact same functionality (e.g., case-sensitive mode: aggregate with GROUP BY executes natively vs. case-sensitive mode: aggregation produces correct results (caseSensitive=true)). Out-of-scope tests in FallbackSuite: FallbackSuite is intended for testing fallback behavior, but many added tests simply validate standard native execution (e.g., generic joins, group-by aggregations) that are already covered by Gluten UTs. Redundancy in IcebergSuite: Multiple tests repeatedly verify basic scan/projection and input_file_name() resolution under caseSensitive=true. Suggestion: Please prune the test suites to focus strictly on the regression scenarios addressed by this PR (i.e. verifying that a user data column named Input_File_Name and the input_file_name() metadata column resolve correctly under spark.sql.caseSensitive = true / false). > > 2. Simplify Collision Handling & Remove Redundant Name Mangling > The GLUTEN_INPUT_FILE_CANON_KEY, column-name mangling (mangledSchemaName), and downstream API changes appear to be unnecessary: > > Redundant mechanism: Whenever a name collision is detected in PushDownInputFileExpression, the query is immediately tagged to fall back to vanilla Spark (addFallbackTag(p)). Since colliding queries never run natively on Velox anyway, introducing mangled names (e.g. **gluten_input_file_col**...), tracking canonical names in Attribute.metadata, and resolving aliases in FileMetadataUtil are all redundant. Invasive API changes: Threading injectedFileColAliases through IteratorApi.genSplitInfo forces unnecessary interface changes across all backends (Velox, ClickHouse, Bolt). Suggestion: Revert the name mangling, GLUTEN_INPUT_FILE_CANON_KEY, and the signature changes in IteratorApi / FileMetadataUtil. > > 3. Handle Fallback Cleanly in doValidateInternal > Rather than manually managing fallback tags and name mangling inside PushDownInputFileExpression, the fallback can be handled naturally in doValidateInternal: > > Separation of concerns: PushDownInputFileExpression should focus purely on pushing down expressions without managing backend schema limitations. Validation in scan transformer: In BasicScanExecTransformer.doValidateInternal(), check whether output contains column name conflicts (e.g., duplicate names under case-insensitive normalization that Velox cannot map simultaneously) and return ValidationResult.failed(...) if so. When validation fails, Gluten's transformer framework will naturally keep the scan on vanilla Spark, and PostOffload will handle the fallback project execution seamlessly. **Test Redundancy & Scope** @jinchengchenghh Thank you for the feedback. The redundant aggregation/GROUP BY tests and the iceberg_cs duplicate tests have been removed in the current revision. The branch now contains exactly two focused regression tests in FallbackSuite: 1. input_file_name() with a user column named Input_File_Name — covers the core regression: caseSensitive=true keeps the scan native, caseSensitive=false falls back, and both return correct data values. 2. user column named like input_file_name is not replaced by the file path — guards against over-eager fallback: when no input_file_name() function is in the query, the scan must stay native regardless of case-sensitivity setting. In IcebergSuite, two new tests cover issue #12712 (getRootPathsInternal always returning empty for Iceberg), which is directly addressed by the getFileFormatAndRootPaths refactor in GlutenIcebergSourceUtil. These are not duplicates of the collision test — they test a distinct bug on the Iceberg path. **Simplify Collision Handling — Remove name mangling / GLUTEN_INPUT_FILE_CANON_KEY / IteratorApi changes** All of this has been reverted. The current implementation contains: 1. No GLUTEN_INPUT_FILE_CANON_KEY or any metadata written to Attribute.metadata 2. No mangledSchemaName or column-name mangling of any kind 3. No changes to IteratorApi.genSplitInfo — the signature is unchanged 4. No changes to FileMetadataUtil.generateMetadataColumns — the signature is unchanged The only new symbol is a single TreeNodeTag[Boolean] (INPUT_FILE_NAME_COLLISION_TAG) placed on the scan node when a collision is detected, and read back in doValidateInternal. **Handle Fallback Cleanly in doValidateInternal** Implemented exactly as suggested. The flow is: 1. PushDownInputFileExpression.PreOffload detects a collision (injected attribute name normalises to the same string as a user data column under ConverterUtils.normalizeColName) and calls p.setTagValue(INPUT_FILE_NAME_COLLISION_TAG, true) on the scan node. 2. ScanTransformerFactory.createFileSourceScanTransformer calls transformer.copyTagsFrom(scanExec) so the tag survives the scan's conversion to a transformer. 3. BasicScanExecTransformer.doValidateInternal reads the tag and returns ValidationResult.failed(...) — the transformer framework then naturally keeps the scan on vanilla Spark, where the file-path thread-local is populated by the JVM iterator. PushDownInputFileExpression itself does not manage any fallback for the collision case — it only sets the tag. The addFallbackTag calls that remain in PreOffload are on the wrapping ProjectExec node, which is pre-existing behaviour unrelated to this fix. -- 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]
