You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Vivek1106-04 (via GitHub)" <gi...@apache.org> on 2026/09/18 17:07:13 UTC
[PR] [SPARK-59317][SQL] Unwrap the cast of a DSv2 scalar subquery runtime filter before pushing it down [spark]
Vivek1106-04 opened a new pull request, #58912:
URL: https://github.com/apache/spark/pull/58912
### What changes were proposed in this pull request?
Type coercion of the compared sides may wrap the filtered column in a cast, e.g.
`cast(part_col as bigint) = <scalar subquery>` when an INT partition column is compared with a
BIGINT scalar subquery. `DataSourceV2Strategy.translateScalarSubqueryFilterV2` literalizes the
scalar subquery once its result is known and translates the comparison with `translateFilterV2`,
which pushes `CAST(part_col AS BIGINT) = <value>` through `V2ExpressionBuilder`. The cast is
pushed but never unwrapped, so a source that only prunes on column references ignores the filter.
The optimizer cannot unwrap it either, because the other side is not a literal until runtime.
This PR adds `UnwrapCastInBinaryComparison.unwrapCastInExpression`, which unwraps the cast of
every binary comparison, `In` and `InSet` of an expression. The rule's own `apply` is rewritten on
top of it, so the optimizer and the runtime path share one function.
`translateScalarSubqueryFilterV2` calls it on the literalized filter:
- when the cast is unwrapped, the comparison is pushed in the column type, e.g. `part_col = 3`;
- when the value is out of the column's range or is rounded by the conversion, the unwrapping
produces a null-returning form that has no V2 predicate of its own, so it is rewritten into what
it filters by before translation: `and(isnull(col), null)` keeps no row and is pushed as `false`
(`AlwaysFalse`), `or(isnotnull(col), null)` keeps the rows where the column is not null and is
pushed as `isnotnull(col)`. Only the whole filter is rewritten, as neither is interchangeable
with its counterpart under a `Not`;
- when the cast cannot be unwrapped, e.g. a lossy one, the filter is pushed as it was.
This is the sibling of SPARK-59301 (#58580), which did the same for the dynamic partition pruning
IN filter. Unlike the IN case, the comparison rule rewrites out-of-range or rounded values into
different comparisons or constant results, hence the handling above.
`InMemoryTableWithV2Filter`, the test table the DSv2 pruning tests use, matched its `=` branch on
any predicate and cast the first child to `FieldReference`, so a pushed cast failed it with a
`ClassCastException`. It now takes that branch only for a reference, as a source that prunes on
column references does.
### Why are the changes needed?
A DSv2 scan whose partition column is compared with a wider-typed scalar subquery received
`CAST(part_col AS BIGINT) = <value>`, which a source pruning on column references cannot use, so
it read every partition. Reported in
https://github.com/apache/spark/pull/58580#issuecomment-5579953790.
Reproduction, with the in-memory V2 table taking runtime filters as V2 predicates: a 10-partition
table with an INT partition column and a BIGINT dimension column.
```sql
SELECT * FROM tbl WHERE part = (SELECT max(val) FROM dim)
```
Before this PR all 10 partitions are read, after it 1 is.
### Does this PR introduce _any_ user-facing change?
Yes. DSv2 sources now receive scalar subquery runtime filters whose column is wrapped in a
lossless numeric cast, so the scan prunes partitions instead of reading all of them. Query results
do not change.
### How was this patch tested?
- New unit tests for `translateScalarSubqueryFilterV2` in `DataSourceV2StrategySuite`: bare and
nested columns, cast unwrapping for `=` and `>`, a value rounded by the conversion, a value
above the column's range for `=` and for `<`, and a lossy cast that must not be unwrapped.
- New end-to-end test in `DataSourceV2SQLSuiteV2Filter`, which asserts the answer and that 1 of 10
partitions survives pruning.
- Control run with the two main-source changes reverted and the tests kept: the unit test pushes
`CAST(cint AS long) = 1` instead of `cint = 1`, and the end-to-end test reads all 10 partitions.
- `UnwrapCastInBinaryComparisonSuite`, `DataSourceV2StrategySuite`, `DataSourceV2SQLSuiteV2Filter`,
`DataSourceV2CatalystRuntimeFilterSuite`, `DynamicPartitionPruningV1Suite` and
`DynamicPartitionPruningV2Suite` pass.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5)
--
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-59317][SQL] Unwrap the cast of a DSv2 scalar subquery runtime filter before pushing it down [spark]
Posted by "Vivek1106-04 (via GitHub)" <gi...@apache.org>.
Vivek1106-04 commented on code in PR #58912:
URL: https://github.com/apache/spark/pull/58912#discussion_r4375621870
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala:
##########
@@ -102,14 +102,48 @@ import org.apache.spark.sql.types._
object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
- case l: LogicalPlan =>
- l.transformExpressionsUpWithPruning(
- _.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
- case e @ (BinaryComparison(_, _) | In(_, _) | InSet(_, _)) => unwrapCast(e).getOrElse(e)
- }
+ case l: LogicalPlan => l.mapExpressions(unwrapCastInExpression(_, ruleId))
+ }
+
+ /**
+ * Unwraps the cast of every binary comparison, `In` and `InSet` of the given expression,
+ * leaving the ones whose cast can't be unwrapped as they are.
+ */
+ private[sql] def unwrapCastInExpression(
+ expr: Expression,
+ ruleId: RuleId = UnknownRuleId): Expression = {
+ expr.transformUpWithPruning(_.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
+ case e @ (BinaryComparison(_, _) | In(_, _) | InSet(_, _)) => unwrapCast(e).getOrElse(e)
+ }
}
- private def unwrapCast(exp: Expression): Option[Expression] = exp match {
+ /**
+ * Unwraps the casts of the given filter, with the comparisons that a value out of the column's
+ * range reduces to returned in a form a data source can express: `FalseLiteral` when no row can
+ * match, and `IsNotNull` when every non-null row matches. The rule returns them as
+ * `and(isnull(col), null)` and `or(isnotnull(col), null)`, which have no source predicate of
+ * their own. The two forms are interchangeable here because a filter only has to tell the rows
+ * it keeps from the ones it drops, and a null result drops the row just like a false one does.
+ *
+ * That equivalence is the filter's and not the expression's, and a `Not` tells the two forms
+ * apart, so the subtrees below one are unwrapped the way the rule does.
+ *
+ * Besides the rule, this is used at runtime to unwrap the cast of a scalar subquery filter,
+ * whose value is only known once its subquery has been evaluated.
+ */
+ private[sql] def unwrapCastInFilter(filter: Expression): Expression = filter match {
+ case And(left, right) => And(unwrapCastInFilter(left), unwrapCastInFilter(right))
+ case Or(left, right) => Or(unwrapCastInFilter(left), unwrapCastInFilter(right))
Review Comment:
Done in 5a03f0f. Added two tests to `DataSourceV2StrategySuite`:
- a disjunction: `cast(cint as bigint) = <out-of-range> OR cint > 100` -> `OR(FALSE, cint > 100)`, and the always-true `<` -> `OR(IS_NOT_NULL(cint), cint > 100)`
- an `In` whose values are all out of the column range -> `AlwaysFalse`, and a date compared with a timestamp that has a non-zero time part -> `AlwaysFalse`
I also checked that the disjunction test fails with the `Or` arm removed.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala:
##########
@@ -102,14 +102,48 @@ import org.apache.spark.sql.types._
object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
- case l: LogicalPlan =>
- l.transformExpressionsUpWithPruning(
- _.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
- case e @ (BinaryComparison(_, _) | In(_, _) | InSet(_, _)) => unwrapCast(e).getOrElse(e)
- }
+ case l: LogicalPlan => l.mapExpressions(unwrapCastInExpression(_, ruleId))
+ }
+
+ /**
+ * Unwraps the cast of every binary comparison, `In` and `InSet` of the given expression,
+ * leaving the ones whose cast can't be unwrapped as they are.
+ */
+ private[sql] def unwrapCastInExpression(
+ expr: Expression,
+ ruleId: RuleId = UnknownRuleId): Expression = {
+ expr.transformUpWithPruning(_.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
+ case e @ (BinaryComparison(_, _) | In(_, _) | InSet(_, _)) => unwrapCast(e).getOrElse(e)
+ }
}
- private def unwrapCast(exp: Expression): Option[Expression] = exp match {
+ /**
+ * Unwraps the casts of the given filter, with the comparisons that a value out of the column's
+ * range reduces to returned in a form a data source can express: `FalseLiteral` when no row can
+ * match, and `IsNotNull` when every non-null row matches. The rule returns them as
+ * `and(isnull(col), null)` and `or(isnotnull(col), null)`, which have no source predicate of
+ * their own. The two forms are interchangeable here because a filter only has to tell the rows
+ * it keeps from the ones it drops, and a null result drops the row just like a false one does.
+ *
+ * That equivalence is the filter's and not the expression's, and a `Not` tells the two forms
+ * apart, so the subtrees below one are unwrapped the way the rule does.
+ *
+ * Besides the rule, this is used at runtime to unwrap the cast of a scalar subquery filter,
Review Comment:
Done in 5a03f0f. The scaladoc now says the rule does not use this and that it must not be used outside filter position, its only use being runtime scalar-subquery filters. It also describes the rewritten comparisons as the ones the rule reduces to a constant for a non-null column (for example out of range, or rounded by the conversion).
--
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-59317][SQL] Unwrap the cast of a DSv2 scalar subquery runtime filter before pushing it down [spark]
Posted by "spark-test (via GitHub)" <gi...@apache.org>.
spark-test commented on code in PR #58912:
URL: https://github.com/apache/spark/pull/58912#discussion_r4346072337
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala:
##########
@@ -102,14 +102,48 @@ import org.apache.spark.sql.types._
object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
- case l: LogicalPlan =>
- l.transformExpressionsUpWithPruning(
- _.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
- case e @ (BinaryComparison(_, _) | In(_, _) | InSet(_, _)) => unwrapCast(e).getOrElse(e)
- }
+ case l: LogicalPlan => l.mapExpressions(unwrapCastInExpression(_, ruleId))
+ }
+
+ /**
+ * Unwraps the cast of every binary comparison, `In` and `InSet` of the given expression,
+ * leaving the ones whose cast can't be unwrapped as they are.
+ */
+ private[sql] def unwrapCastInExpression(
+ expr: Expression,
+ ruleId: RuleId = UnknownRuleId): Expression = {
+ expr.transformUpWithPruning(_.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
+ case e @ (BinaryComparison(_, _) | In(_, _) | InSet(_, _)) => unwrapCast(e).getOrElse(e)
+ }
}
- private def unwrapCast(exp: Expression): Option[Expression] = exp match {
+ /**
+ * Unwraps the casts of the given filter, with the comparisons that a value out of the column's
+ * range reduces to returned in a form a data source can express: `FalseLiteral` when no row can
+ * match, and `IsNotNull` when every non-null row matches. The rule returns them as
+ * `and(isnull(col), null)` and `or(isnotnull(col), null)`, which have no source predicate of
+ * their own. The two forms are interchangeable here because a filter only has to tell the rows
+ * it keeps from the ones it drops, and a null result drops the row just like a false one does.
+ *
+ * That equivalence is the filter's and not the expression's, and a `Not` tells the two forms
+ * apart, so the subtrees below one are unwrapped the way the rule does.
+ *
+ * Besides the rule, this is used at runtime to unwrap the cast of a scalar subquery filter,
Review Comment:
**Nit (P3):** This sentence was carried over from the `unwrapCastInSet` comment, where the rule does call that method. Here the rule's `apply` only calls `unwrapCastInExpression`, and the only caller of `unwrapCastInFilter` is `DataSourceV2Strategy.translateScalarSubqueryFilterV2`. Since the filter-only substitution (`FalseLiteral` / `IsNotNull`) is unsound outside filter position, a doc that suggests the optimizer rule uses it could lead a later change to call it from the wrong place.
The opening sentence also limits the rewrite to 'a value out of the column's range'. Under `filterSemantics`, a rounded `EqualTo` value, an `In`/`InSet` whose values all fail to cast, and a timestamp-to-date comparison with a non-zero time part also become `FalseLiteral`. The suite's own `3.14d` case expects `AlwaysFalse`. Could you say that this is only used for runtime scalar-subquery filters, and describe the rewritten comparisons as the ones the rule reduces to a constant for a non-null column (for example out of range or rounded by the conversion)?
**Verification:**
- **Inspection:** The scaladoc states that unwrapCastInFilter's only use is runtime scalar-subquery filter translation, and its description of the rewritten comparisons covers rounded values as well as out-of-range ones.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/UnwrapCastInBinaryComparison.scala:
##########
@@ -102,14 +102,48 @@ import org.apache.spark.sql.types._
object UnwrapCastInBinaryComparison extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = plan.transformWithPruning(
_.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
- case l: LogicalPlan =>
- l.transformExpressionsUpWithPruning(
- _.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
- case e @ (BinaryComparison(_, _) | In(_, _) | InSet(_, _)) => unwrapCast(e).getOrElse(e)
- }
+ case l: LogicalPlan => l.mapExpressions(unwrapCastInExpression(_, ruleId))
+ }
+
+ /**
+ * Unwraps the cast of every binary comparison, `In` and `InSet` of the given expression,
+ * leaving the ones whose cast can't be unwrapped as they are.
+ */
+ private[sql] def unwrapCastInExpression(
+ expr: Expression,
+ ruleId: RuleId = UnknownRuleId): Expression = {
+ expr.transformUpWithPruning(_.containsAnyPattern(BINARY_COMPARISON, IN, INSET), ruleId) {
+ case e @ (BinaryComparison(_, _) | In(_, _) | InSet(_, _)) => unwrapCast(e).getOrElse(e)
+ }
}
- private def unwrapCast(exp: Expression): Option[Expression] = exp match {
+ /**
+ * Unwraps the casts of the given filter, with the comparisons that a value out of the column's
+ * range reduces to returned in a form a data source can express: `FalseLiteral` when no row can
+ * match, and `IsNotNull` when every non-null row matches. The rule returns them as
+ * `and(isnull(col), null)` and `or(isnotnull(col), null)`, which have no source predicate of
+ * their own. The two forms are interchangeable here because a filter only has to tell the rows
+ * it keeps from the ones it drops, and a null result drops the row just like a false one does.
+ *
+ * That equivalence is the filter's and not the expression's, and a `Not` tells the two forms
+ * apart, so the subtrees below one are unwrapped the way the rule does.
+ *
+ * Besides the rule, this is used at runtime to unwrap the cast of a scalar subquery filter,
+ * whose value is only known once its subquery has been evaluated.
+ */
+ private[sql] def unwrapCastInFilter(filter: Expression): Expression = filter match {
+ case And(left, right) => And(unwrapCastInFilter(left), unwrapCastInFilter(right))
+ case Or(left, right) => Or(unwrapCastInFilter(left), unwrapCastInFilter(right))
Review Comment:
**Nit (P3):** The Or arm here, and the `filterSemantics` argument passed to the In/InSet arms and to `unwrapTimestampToDate`, are not exercised by any of the new tests. The `translateScalarSubqueryFilterV2` cases in `DataSourceV2StrategySuite` cover a bare comparison, `And` and `Not`, and only numeric casts (int to long, float to double).
The PR description explicitly claims that `cast(part as bigint) = <out-of-range> OR part > 100` is pushed as `OR(FALSE, part > 100)`, and that disjunction is the form from the earlier review. If this line were dropped, the `Or` would fall through to `case other => unwrapCastInExpression(other)` and be pushed as `Or(And(IsNull(part), null), part > 100)`, with a `BOOLEAN_EXPRESSION(null)` child. No test would fail. The same is true for an `In` whose values all fall outside the column type, and for a DATE column compared with a TIMESTAMP that has a non-zero time part.
Results stay correct because the filter is re-evaluated above the scan, but the pruning would be lost without any signal. Could you add cases for a disjunction (both an `AlwaysFalse` and an `IS_NOT_NULL` operand), an all-out-of-range `In`, and a date/timestamp comparison?
**Verification:**
- **Regression:** A disjunction containing a cast comparison with an out-of-range scalar-subquery value translates to an OR whose operand is AlwaysFalse (or IS_NOT_NULL for an always-true comparison), not a predicate with a null boolean child.
- **Behavior:** An In over a cast column whose literal values are all outside the column type, and a date column compared with a timestamp that has a non-zero time part, each translate to AlwaysFalse.
--
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-59317][SQL] Unwrap the cast of a DSv2 scalar subquery runtime filter before pushing it down [spark]
Posted by "Vivek1106-04 (via GitHub)" <gi...@apache.org>.
Vivek1106-04 commented on PR #58912:
URL: https://github.com/apache/spark/pull/58912#issuecomment-5773198638
Thank you for the review @uros-b. I went with the second option: unwrap now returns a form a data source can express, so `DataSourceV2Strategy` no longer matches the rule's encoding.
`UnwrapCastInBinaryComparison` gains a second entry point, `unwrapCastInFilter`, and a `filterSemantics` flag threaded through `unwrapCast`. Under it `falseIfNotNull(e)` returns `FalseLiteral` and `trueIfNotNull(e)` returns `IsNotNull(e)`, so unwrapping a filter yields only `EqualTo`/`GreaterThan`/`FalseLiteral`/`IsNotNull` and the like. The flag defaults to false, so the rule's own path is unchanged. Both encodings now stay in the file that defines them.
`unwrapCastInFilter` walks the `And`/`Or` tree rather than the root only, and leaves the subtrees below a `Not` to the plain unwrapping. The substitution is the filter's equivalence, not the expression's: a filter only has to tell the rows it keeps from the ones it drops, and within a `Not`-free `And`/`Or` tree a null result drops the row exactly like a false one (`null AND true` and `false AND true` both drop; `null OR false` and `false OR false` both drop). `Not` inverts that, hence the carve-out.
A disjunction over the partition column reaches this too, e.g.
```sql
WHERE part = (SELECT max(val) FROM dim) OR part > 100
```
which unwrapped to `Or(And(IsNull(part), null), part > 100)` and was pushed with the `BOOLEAN_EXPRESSION(null)` child you describe. Walking the tree covers it, and I added your tests as written:
- `EqualTo(Cast(cint, LongType), Literal(Int.MaxValue + 1L)) AND other` -> `AND(FALSE, other)`
- `LessThan(Cast(cint, LongType), Literal(Int.MaxValue + 1L)) AND other` -> `AND(IS_NOT_NULL(cint), other)`
- `NOT (EqualTo(Cast(cint, LongType), Literal(Int.MaxValue + 1L)))` is translated, is not `NOT(FALSE)`, and matches what the rule's own unwrapping translates to
`UnwrapCastInBinaryComparisonSuite` and `DataSourceV2StrategySuite` pass, along with the end-to-end pruning test. PR description updated.
--
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-59317][SQL] Unwrap the cast of a DSv2 scalar subquery runtime filter before pushing it down [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #58912:
URL: https://github.com/apache/spark/pull/58912#issuecomment-6412044256
Please address the few minor comments, and resolve conflicts!
--
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-59317][SQL] Unwrap the cast of a DSv2 scalar subquery runtime filter before pushing it down [spark]
Posted by "Vivek1106-04 (via GitHub)" <gi...@apache.org>.
Vivek1106-04 commented on PR #58912:
URL: https://github.com/apache/spark/pull/58912#issuecomment-6412428397
Thank you @uros-b. Addressed both nits in 5a03f0f and rebased on master to resolve the conflict (upstream widened `simplifyNumericComparison` to `TimeType`; the new branches thread `filterSemantics` too). `UnwrapCastInBinaryComparisonSuite` and `DataSourceV2StrategySuite` pass.
cc @cloud-fan, could you take a look as well?
--
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