felipepessoto commented on PR #12626: URL: https://github.com/apache/gluten/pull/12626#issuecomment-5374630265
I have the same understanding, and that's exactly the premise of this PR, thanks for confirming. [`PushDownFilterToScan`](https://github.com/apache/gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/gluten-substrait/src/main/scala/org/apache/gluten/extension/columnar/PushDownFilterToScan.scala#L30-L46) pushes *every* conjunct into the native scan, and [`FilterExecTransformerBase.isNoop`](https://github.com/apache/gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/gluten-substrait/src/main/scala/org/apache/gluten/execution/BasicPhysicalOperatorTransformer.scala#L61) (`getRemainingCondition == null`, where [`getRemainingCondition`](https://github.com/apache/gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/gluten-substrait/src/main/scala/org/apache/gluten/execution/BasicPhysicalOperatorTransformer.scala#L107-L123) subtracts the scan's filters from the filter condition) then makes the paired `FilterExecTransformer` a no-op, so nothing re-evaluates those predicates. Spark's convention for "the source fully handles this filter, no post-scan `Filter` needed" is the leading `*` on `PushedFilters`, see [`RowDataSourceScanExec.metadata`](https://github.com/apache/spark/blob/7c29c664cdc9321205a98a14858aaf8daaa19db2/sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala#L137-L143), which prefixes each `handledFilters` entry with `*`. Gluten instead inherits [`FileSourceScanLike.metadata`](https://github.com/apache/spark/blob/7c29c664cdc9321205a98a14858aaf8daaa19db2/sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala#L398), which renders `PushedFilters` unmarked. That's the right call for vanilla Spark, where Parquet/ORC pushdown is only best-effort row-group/page pruning, so the `Filter` above must re-check every row, but for Gluten it under-reports what the native scan actually does: ``` PushedFilters: [IsNotNull(id), LessThan(id,5)] <- Gluten today PushedFilters: [*IsNotNull(id), *LessThan(id,5)] <- what the state actually is ``` So this is a rendering-only change; execution behavior is untouched. ## Why the change isn't unconditional Worth calling out, because it's the one place where "all pushed filters are fully evaluated" does *not* hold: the property depends on the backend opting into Gluten's full pushdown via [`SparkPlanExecApi.supportPushDownFilterToScan`](https://github.com/apache/gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/SparkPlanExecApi.scala#L571) (default `true`). ClickHouse deliberately returns `false` for Parquet ([`CHSparkPlanExecApi.supportPushDownFilterToScan`](https://github.com/apache/gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/backends-clickhouse/src/main/scala/org/apache/gluten/backendsapi/clickhouse/CHSparkPlanExecApi.scala#L877-L898), "Let's make push down functionally same as vanilla Spark for now"). In that case `pushDownFilters` is `None`, so [`BasicScanExecTransformer.filterExprs()`](https://github.com/apache/gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/gluten-substrait/src/main/scala/org/apache/gluten/execution/BasicScanExecTransformer.scala#L41-L62) takes the `else` branch and silently keeps only the [`isSupportedScanFilter`](https://github.com/apache/gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/gluten-substrait/src/main/scala/org/apache/gluten/backendsapi/SparkPlanExecApi.scala#L574-L580) subset (note the `todo` there), and [`CHFilterExecTransformer.getRemainingCondition`](https://github.com/apache /gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/backends-clickhouse/src/main/scala/org/apache/gluten/execution/CHFilterExecTransformer.scala#L26-L42) leaves a real, non-no-op `Filter` above the scan. Marking there would be a false claim, so the PR guards on [`supportPushDownFilterToScan(this)`](https://github.com/apache/gluten/blob/5587af879e7facfbd0d91af4656e1c10c9aff6d3/gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala#L220-L228). Vanilla `FileSourceScanExec` nodes (AQE *Initial Plan*, fallback) also stay unmarked, since they still have a real `Filter` above them. ## Implementation note I'd like your opinion on To be precise about the constraint, since "can't be overridden" would be wrong: `metadata` *is* overridable, and [`FileSourceScanLike`](https://github.com/apache/spark/blob/7c29c664cdc9321205a98a14858aaf8daaa19db2/sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala#L386) already overrides [`DataSourceScanExec.metadata`](https://github.com/apache/spark/blob/7c29c664cdc9321205a98a14858aaf8daaa19db2/sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala#L56) that way. What is not possible is *decorating* it, because it is a `lazy val`: `override lazy val metadata = super.metadata + ...` fails to compile with `super may not be used on lazy value metadata`, and the stackable `abstract override lazy val` variant fails with the same error (overriding it with a strict `val` is rejected too, it must stay lazy). So an override cannot reuse Spark's map, it has to rebuild the whole thing (`Format`, `ReadSchema`, `Batched`, `PartitionFilters `, `PushedFilters`, `DataFilters`, `Location`) from scratch, and that body drifts between Spark versions, so we would be copying and then maintaining it in all five shim modules. Overriding [`pushedDownFilters`](https://github.com/apache/spark/blob/7c29c664cdc9321205a98a14858aaf8daaa19db2/sql/core/src/main/scala/org/apache/spark/sql/execution/DataSourceScanExec.scala#L374) instead is not an option either, since the same value is handed to `buildReaderWithPartitionValues` for the row-based path, so a display-only tweak there would corrupt real pushdown. That is why I apply the `*` to the rendered node string in both paths that print it: - [`simpleString`](https://github.com/apache/gluten/blob/5587af879e7facfbd0d91af4656e1c10c9aff6d3/gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala#L230-L244) (`executedPlan.toString`), and - [`verboseStringWithOperatorId`](https://github.com/apache/gluten/blob/5587af879e7facfbd0d91af4656e1c10c9aff6d3/gluten-substrait/src/main/scala/org/apache/gluten/execution/FileSourceScanExecTransformer.scala#L249-L256) (`FormattedMode` / plan-stability goldens). It works, and the golden diff is purely the `*` toggling (0 vanilla `Scan parquet` nodes marked, 0 `FileSourceScanExecTransformer` nodes left unmarked across all 1672 golden files). But if you'd prefer a per-shim `override lazy val metadata` that restates Spark's map instead, with the maintenance cost that implies, or think this is worth an upstream Spark change (e.g. making the `PushedFilters` entry come from an overridable hook), I'm happy to redo it that way. ## Practical payoff Beyond the plan string being accurate, upstream suites that assert the `*`-marked form start passing, namely Delta's CDF filter-pushdown tests, which we can then drop from [`known-failures.txt`](https://github.com/apache/gluten/blob/f5764eb1bb59b791ad022f25ed3b71acf63d26da/.github/workflows/util/delta-spark-ut/known-failures.txt#L74-L86) (tracked in #12753). -- 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]
