peter-toth commented on code in PR #58145:
URL: https://github.com/apache/spark/pull/58145#discussion_r4148253997
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/execution/datasources/v2/DataSourceV2Relation.scala:
##########
@@ -361,8 +419,10 @@ case class DataSourceV2ScanRelation(
.map(o => o.copy(child = QueryPlan.normalizeExpressions(o.child,
output)))
),
// pushedFilters may reference columns pruned out of `output` (see the
field doc), so they are
- // normalized against the relation's full output rather than `output`.
- pushedFilters = pushedFilters.map(QueryPlan.normalizeExpressions(_,
relation.output))
+ // normalized against the relation's full output rather than `output`.
The same holds for
+ // inferredFilters.
Review Comment:
**Finding 17.** This says inferred filters may reference columns pruned out
of `output`, like `pushedFilters`. They can't. `pruneColumns` keeps only the
ones whose references are a subset of `output`
(`V2ScanRelationPushDown.scala:1028`). Normalizing against `relation.output`
still works, so only the comment needs fixing.
```suggestion
// pushedFilters may reference columns pruned out of `output` (see the
field doc), so they are
// normalized against the relation's full output rather than `output`.
inferredFilters only
// reference `output` (pruneColumns drops the rest), so the same
normalization is safe.
```
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/V2ScanRelationPushDown.scala:
##########
@@ -1261,6 +1292,78 @@ object V2ScanRelationPushDown extends Rule[LogicalPlan]
with PredicateHelper {
}
}
+ private def shouldAddSparkPostPushdownAdjustmentFilters(scan: Scan): Boolean
= scan match {
+ case s: SupportsReportStatistics => !s.reflectsFullyPushedDownFilters()
+ case _ => false
+ }
+
+ private def getInferredFilters(sHolder: ScanBuilderHolder): Seq[Expression]
= {
+ sHolder.builder match {
+ case r: SupportsPushDownCatalystFilters =>
+ val validFilters = r.inferredFilters
+ .flatMap(rebindInferredFilter(_, sHolder.output))
+ .filter(validateInferredFilter(_, sHolder.output))
+ .flatMap(splitConjunctivePredicates)
+ ExpressionSet(validFilters).toSeq
+ case _ =>
+ Nil
+ }
+ }
+
+ private def validateInferredFilter(
+ filter: Expression,
+ output: Seq[AttributeReference]): Boolean = {
+ val hasInvalidColumnReference = filter.exists {
+ case _: AttributeReference => false
+ case _: Attribute | _: BoundReference | _: LateralColumnAliasReference |
+ _: OuterReference | _: OuterScopeReference => true
+ case _ => false
+ }
+ val hasValidStructure = filter.deterministic &&
+ !SubqueryExpression.hasSubquery(filter) &&
+ !filter.containsPattern(EXTERNAL_UDF) &&
+ !filter.exists(_.isInstanceOf[UserDefinedExpression]) &&
Review Comment:
You're right. The one-liner I suggested rejects every filter, because
`AttributeReference` is `Unevaluable`. I should have run it before proposing it.
This version works on this head:
```scala
val hasUnevaluableExpression = filter.exists {
case _: AttributeReference => false
case _: Unevaluable | _: RuntimeReplaceable => true
case _ => false
}
```
plus `!hasUnevaluableExpression &&` in `hasValidStructure`.
`RuntimeReplaceable` needs to be in there too. `ReplaceExpressions` and
`RewriteWithExpression` run in the first optimizer batches, long before
`V2ScanRelationPushDown`. So a reported `Nvl` or `Between` is never replaced,
and codegen throws on it.
I checked with a source that returns every row and reports one inferred
filter on `j` (every returned row has `j` in [-9, 0]), under `WHERE i > 2`:
| Inferred filter | Default path | Opted in |
|---|---|---|
| `j < 100` | 7 rows | 7 rows |
| `nvl(j, 0) < 100` | `[INTERNAL_ERROR] Cannot generate code for expression:
nvl(j#38, 0)` | 7 rows |
| `between(j, -100, 100)` | `[INTERNAL_ERROR] Cannot generate code for
expression: between(j#74, -100, 100)` | 7 rows |
With the guard above, all six return the 7 rows. `nvl` and `between` are
dropped with the existing "Ignoring invalid inferred filter" warning. The 29
`inferred filter` tests in `DataSourceV2StrategySuite` and `DataSourceV2Suite`
stay green.
Opted-in scans never evaluate the inferred predicate, so only the default
path fails. Could the `TimeWindow` regression also cover `Nvl` and `Between`?
--
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]
