You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "szehon-ho (via GitHub)" <gi...@apache.org> on 2026/07/17 18:25:09 UTC
[PR] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan [spark]
szehon-ho opened a new pull request, #57340:
URL: https://github.com/apache/spark/pull/57340
### What changes were proposed in this pull request?
This PR adds an optional `fullyPushedFilters` hint to the `FileIndex` trait and
teaches `FileSourceStrategy` to use it when deciding which filters can be removed
from the row-level `FilterExec` placed above the scan.
- `FileIndex.fullyPushedFilters: Option[Seq[Expression]]` defaults to `None`.
- In `FileSourceStrategy`, the set of post-scan (after-scan) filters is now computed as:
- When the `FileIndex` returns `Some(fullyPushedFilters)`, those filters are the
only basis for removing predicates from the `FilterExec` above the scan
(`filterSet -- fullyPushedFilters`).
- When it returns `None`, planning keeps the existing default behavior
(`filterSet -- partitionKeyFilters.filter(_.references.nonEmpty)`).
This lets a `FileIndex` implementation that guarantees certain predicates for all
returned rows (e.g. an index that fully prunes on non-partition columns) tell the
planner exactly which filters are safe to drop, instead of relying only on the
default partition-key heuristic.
### Why are the changes needed?
Today `FileSourceStrategy` assumes only partition-key filters are fully satisfied by
file listing, so any other predicate a `FileIndex` may fully guarantee must still be
re-evaluated in a `FilterExec`. Custom `FileIndex` implementations have no way to
communicate stronger pushdown guarantees to the physical plan. The new hint provides
an opt-in mechanism, with no behavior change for existing indexes (default `None`).
### Does this PR introduce _any_ user-facing change?
No. The new trait method has a default implementation returning `None`, which
preserves the existing planning behavior for all built-in `FileIndex` implementations.
### How was this patch tested?
Added a unit test in `FileSourceStrategySuite`
("FileIndex fullyPushedFilters controls final FilterExec removal") that exercises a
test `FileIndex` returning `None`, `Some(Nil)`, and `Some(<data filter>)`, and asserts
the resulting `FilterExec` predicates. Ran:
build/sbt 'sql/testOnly *FileSourceStrategySuite -- -z "fullyPushedFilters"'
All tests passed.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 4.8)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan [spark]
Posted by "github-actions[bot] (via GitHub) via reviews" <re...@spark.apache.org>.
github-actions[bot] commented on PR #57340:
URL: https://github.com/apache/spark/pull/57340#issuecomment-6566172377
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #57340:
URL: https://github.com/apache/spark/pull/57340#discussion_r3606285906
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileSourceStrategy.scala:
##########
@@ -213,7 +213,12 @@ object FileSourceStrategy extends Strategy with PredicateHelper with Logging {
logInfo(log"Pushed Filters: ${MDC(PUSHED_FILTERS, pushedFilters.mkString(","))}")
// Predicates with both partition keys and attributes need to be evaluated after the scan.
- val afterScanFilters = filterSet -- partitionKeyFilters.filter(_.references.nonEmpty)
+ // When the FileIndex reports explicit full-pushdown guarantees, use them as the only basis
+ // for removing filters from the row-level FilterExec above the scan.
+ val afterScanFilters = fsRelation.location.fullyPushedFilters match {
+ case Some(fullyPushedFilters) => filterSet -- fullyPushedFilters
+ case None => filterSet -- partitionKeyFilters.filter(_.references.nonEmpty)
Review Comment:
yes, let me address with the other concern (that we dont call listFiles yet until physical execution..)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #57340:
URL: https://github.com/apache/spark/pull/57340#discussion_r3618875362
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileIndex.scala:
##########
@@ -133,6 +133,35 @@ trait FileIndex {
def listFiles(
partitionFilters: Seq[Expression], dataFilters: Seq[Expression]): Seq[PartitionDirectory]
+ /**
+ * Given the `partitionKeyFilters` and remaining `dataFilters` Spark will use to prune
+ * files for this index, returns the filters that this index guarantees are fully applied for all
+ * returned rows. Only these are dropped from the row-level `FilterExec` above the
+ * scan; any filter not returned is still evaluated after the scan for correctness. By default
+ * only partition-key filters are considered fully pushed, preserving the standard file-source
+ * behavior.
+ *
+ * For example:
+ * - An index that fully applies additional predicates (e.g. one that prunes on non-partition
+ * columns) can override this to also return a subset of `dataFilters`.
+ * - Conversely, an index must omit a filter that it only partially applies. With partition
+ * evolution, not all data files are partitioned by a given partition column (e.g. older files
+ * written under a previous partition spec). Such files may contain both matching and
+ * non-matching rows for that column, so the partition filter is not fully pushed and must be
+ * omitted.
+ *
+ * Note these filters are the same predicates that [[listFiles]] prunes with, but not necessarily
Review Comment:
I will have to think a bit more.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #57340:
URL: https://github.com/apache/spark/pull/57340#discussion_r3618873312
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileIndex.scala:
##########
@@ -133,6 +133,35 @@ trait FileIndex {
def listFiles(
partitionFilters: Seq[Expression], dataFilters: Seq[Expression]): Seq[PartitionDirectory]
+ /**
+ * Given the `partitionKeyFilters` and remaining `dataFilters` Spark will use to prune
+ * files for this index, returns the filters that this index guarantees are fully applied for all
+ * returned rows. Only these are dropped from the row-level `FilterExec` above the
+ * scan; any filter not returned is still evaluated after the scan for correctness. By default
+ * only partition-key filters are considered fully pushed, preserving the standard file-source
+ * behavior.
+ *
+ * For example:
+ * - An index that fully applies additional predicates (e.g. one that prunes on non-partition
+ * columns) can override this to also return a subset of `dataFilters`.
+ * - Conversely, an index must omit a filter that it only partially applies. With partition
+ * evolution, not all data files are partitioned by a given partition column (e.g. older files
+ * written under a previous partition spec). Such files may contain both matching and
+ * non-matching rows for that column, so the partition filter is not fully pushed and must be
+ * omitted.
+ *
+ * Note these filters are the same predicates that [[listFiles]] prunes with, but not necessarily
Review Comment:
This statement worries me. Like I don't know if this is any better that having a simple boolean flag to indicate if FileIndex enforces / pushes down all partition predicates.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #57340:
URL: https://github.com/apache/spark/pull/57340#discussion_r3626452229
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileIndex.scala:
##########
@@ -133,6 +133,35 @@ trait FileIndex {
def listFiles(
partitionFilters: Seq[Expression], dataFilters: Seq[Expression]): Seq[PartitionDirectory]
+ /**
+ * Given the `partitionKeyFilters` and remaining `dataFilters` Spark will use to prune
+ * files for this index, returns the filters that this index guarantees are fully applied for all
+ * returned rows. Only these are dropped from the row-level `FilterExec` above the
+ * scan; any filter not returned is still evaluated after the scan for correctness. By default
+ * only partition-key filters are considered fully pushed, preserving the standard file-source
+ * behavior.
+ *
+ * For example:
+ * - An index that fully applies additional predicates (e.g. one that prunes on non-partition
+ * columns) can override this to also return a subset of `dataFilters`.
+ * - Conversely, an index must omit a filter that it only partially applies. With partition
+ * evolution, not all data files are partitioned by a given partition column (e.g. older files
+ * written under a previous partition spec). Such files may contain both matching and
+ * non-matching rows for that column, so the partition filter is not fully pushed and must be
+ * omitted.
+ *
+ * Note these filters are the same predicates that [[listFiles]] prunes with, but not necessarily
Review Comment:
discussed offline, let's remove the dataFilter argument for now as it transforms more before listFiles(). Just passing list of Partition filters is more simple.
Also renamed the method
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan [spark]
Posted by "yyanyy (via GitHub)" <gi...@apache.org>.
yyanyy commented on code in PR #57340:
URL: https://github.com/apache/spark/pull/57340#discussion_r3605370968
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/FileSourceStrategy.scala:
##########
@@ -213,7 +213,12 @@ object FileSourceStrategy extends Strategy with PredicateHelper with Logging {
logInfo(log"Pushed Filters: ${MDC(PUSHED_FILTERS, pushedFilters.mkString(","))}")
// Predicates with both partition keys and attributes need to be evaluated after the scan.
- val afterScanFilters = filterSet -- partitionKeyFilters.filter(_.references.nonEmpty)
+ // When the FileIndex reports explicit full-pushdown guarantees, use them as the only basis
+ // for removing filters from the row-level FilterExec above the scan.
+ val afterScanFilters = fsRelation.location.fullyPushedFilters match {
+ case Some(fullyPushedFilters) => filterSet -- fullyPushedFilters
+ case None => filterSet -- partitionKeyFilters.filter(_.references.nonEmpty)
Review Comment:
nit/nonblocker - I wonder if we can always set partition key filters in index when we see it the first time to avoid the fallback; is that possible?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan [spark]
Posted by "github-actions[bot] (via GitHub) via reviews" <re...@spark.apache.org>.
github-actions[bot] closed pull request #57340: [WIP] [SPARK-58199][SQL] Add FileIndex fullyPushedFilter hint for physical plan
URL: https://github.com/apache/spark/pull/57340
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org