sabbasani commented on PR #12726: URL: https://github.com/apache/gluten/pull/12726#issuecomment-6309258363
> 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. Reply to the top-level review (the three numbered points): Thanks for the review, all three points are addressed in da76d93e3. 1. Tests: I removed the duplicate and out-of-scope tests from FallbackSuite and IcebergSuite (the aggregation, GROUP BY, join and projection tests, and the repeated input_file_name() tests). What remains is: 2. FallbackSuite: one test for a user column Input_File_Name alongside input_file_name() under caseSensitive=true/false, plus one test that a non-conflicting query stays native. IcebergSuite: one test for the same scenario under both settings. Name mangling and API changes: I reverted mangledSchemaName, GLUTEN_INPUT_FILE_CANON_KEY, the metadata on injected attributes, and injectedFileColAliases. IteratorApi.genSplitInfo, the Velox and Bolt implementations, FileMetadataUtil and BasicScanExecTransformer are back to main. In CHIteratorApi I only reverted the alias parameter; its resolver fix stays. 3. doValidateInternal(): I tried this first, but it can't see the conflict. I logged output inside doValidateInternal() for SELECT id, Input_File_Name, input_file_name() with caseSensitive=false. It only contains id,Input_File_Name. PostOffload adds the injected input_file_name attribute after offload, so a check in doValidateInternal() passes and Velox then fails at run time with Cannot map from same table column to different outputs in table scan. So the check is now a single FallbackTags.add in PreOffload.addMetadataCol, where both sets of names are visible. It uses ConverterUtils.normalizeColName, so caseSensitive=true still runs natively. If you'd rather have it in the scan, PostOffload would need to revalidate the scan and revert it to vanilla Spark. I think that is more than this PR needs, but I can do it if you prefer. -- 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]
