malinjawi commented on PR #12218: URL: https://github.com/apache/gluten/pull/12218#issuecomment-5203220912
@zhztheplayer You were right, and I've switched to it — `2472fd844` replaces `DeltaCDFScanStrategy` with `DeltaCDFScanRule`, injected at post-hoc resolution. To be precise about why a *physical* pre-transform rule can't work: by then Delta has already collapsed `DeltaCDFRelation` into a `RowDataSourceScanExec` whose file scans live inside the RDD. But a **logical** rule does work, and it's simpler. Running before the optimizer means only an output-restoring projection is needed — Spark then pushes the surrounding predicates into the expanded file scans itself. That removes the `PhysicalOperation` matching, the filter/project rewrites, and the out-of-band `sessionState.optimizer.execute` call: ~110 lines down to ~50. It also removes a real failure mode. Expanding after the optimizer meant hand-preserving expression IDs against already-planned operators; when I test the earlier form of that logic it dies with `IllegalStateException: Couldn't find <attr>` at shuffle binding — on Spark 3.5 as well as 3.3, so it wasn't the version-specific quirk the old comment claimed. The logical rule has no equivalent failure mode. Verified against Spark 3.5.5 / Delta 3.3.2 and Spark 3.3.1 / Delta 2.3.0 — 15 query shapes plus the DataFrame-API paths, AQE on and off, matching vanilla Delta including error behavior, with identical predicate pushdown. Expansion is also wrapped now so any failure leaves Delta's own CDF path intact — the rule runs during analysis, where an escaping exception would otherwise fail the query outright, including for metadata-only calls such as `explain()`. CI is green apart from `spark-test-spark40 (2)`, which is the `GlutenSparkScriptTransformationSuite` SIGSEGV in `libgluten.so` currently hitting unrelated PRs (#12690, #12692, #12695, #12697) and passing on others — it reproduces on this branch both before and after this change. cc @felipepessoto — the implementation changed since your approval, so it's worth re-syncing #12582 and rerunning the combined Delta CI (last run there was July 28). The behavior it validated is unchanged and the standard CI Delta suites pass. -- 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]
