sarutak commented on code in PR #59940:
URL: https://github.com/apache/spark/pull/59940#discussion_r4429291189
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/FilterPushdownSuite.scala:
##########
@@ -750,6 +750,314 @@ class FilterPushdownSuite extends PlanTest {
assertTopFilterConjuncts(Optimize.execute(originalQuery),
Seq(nonDeterministic, unplaced))
}
+ test("SPARK-60730: a cheap condition that can fail is not pushed ahead of an
expensive one") {
+ Seq(true, false).foreach { split =>
+ withSQLConf(SQLConf.SPLIT_PROJECTION_FOR_EXPENSIVE_FILTERS.key ->
split.toString) {
+ val originalQuery = testStringRelation
+ .select($"a", $"e".rlike("magic") as "f", $"b")
Review Comment:
Good point. The new tests now use `expensiveRLike(tag)`, with the same
definition and at the same place as in #59879, so they hold with either
heuristic. The Python UDF test uses a pattern that is not a constant too.
`evaluatedHere` now replaces a Python UDF with an attribute, as
`ExtractPythonUDFs` does, instead of a NULL literal, so `e RLIKE py(a)` stays
expensive with #59879's `!right.foldable`. So I do not think the two PRs need
an order. Until #59879 lands, every `RLIKE` alias over a column counts as
expensive, as before this PR, so the hold-back applies to those aliases too.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/ExprUtils.scala:
##########
@@ -266,13 +269,22 @@ object ExprUtils extends EvalHelper with QueryErrorsBase {
e.deterministic && !e.containsPattern(PLAN_EXPRESSION) &&
canEvaluateUnconditionallyInternal(e)
+ // Only the numeric pairs are taken from `Cast.canUpCast`, which also covers
e.g. LONG to
+ // TIMESTAMP and casts to STRING or between complex types: widening a number
never fails.
+ private def isNumericUpCast(from: DataType, to: DataType): Boolean = (from,
to) match {
+ case (_: NumericType, _: NumericType) => Cast.canUpCast(from, to)
+ case _ => false
+ }
+
private def canEvaluateUnconditionallyInternal(e: Expression): Boolean = e
match {
case _: AttributeReference | _: Literal => true
case _: GetStructField | _: GetArrayStructFields | _: GetMapValue =>
e.children.forall(canEvaluateUnconditionallyInternal)
case _: And | _: Or | _: Not | _: BinaryComparison | _: IsNull | _:
IsNotNull |
- _: IsNaN | _: NullIf | _: Coalesce | _: In | _: InSet =>
+ _: IsNaN | _: NullIf | _: Coalesce | _: In | _: InSet | _: Size =>
e.children.forall(canEvaluateUnconditionallyInternal)
Review Comment:
Added `StartsWith`, `EndsWith` and `Contains`, but not `Like` or `RLike`,
and also `Length` and `OctetLength`, since `LikeSimplification` turns `LIKE
'ab%c'` into `octet_length(s) >= 3 AND StartsWith(s, 'ab') AND EndsWith(s,
'c')` (`length` with `UTF8_LCASE`), which was still rejected; so every `LIKE`
without `_` over a column that it simplifies is pushed now. They can not fail
for any collation: the ICU path returns before searching for an empty string,
and an `ExprUtilsSuite` test evaluates them over binary, lowercase, ICU and
trim collations on empty, ignorable and invalid UTF-8 strings. A
`GetStructField` over a `CreateNamedStruct` is accepted when every field of the
struct passes, since the whole struct is evaluated until
`SimplifyExtractValueOps` reduces it, and that rule can be excluded. So your
`struct(a, b) AS s` with `s.a > 1` is pushed now, while a struct with a field
that can fail, e.g. `struct(id AS a, id * 2 AS b)`, stays rejected, since `id *
2` can overflow in ANSI
mode. `FilterExec`, `StreamedSideJoinCondition` and
`RewriteDistinctAggregates` accept them too; their suites and the TPC-DS plan
stability suites pass with no plan changes.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala:
##########
@@ -2652,8 +2681,20 @@ object PushPredicateThroughNonJoin extends
Rule[LogicalPlan] with PredicateHelpe
// non-deterministic expression sees. Throwable conditions, or
conditions over throwable
// aliases, stay there too: ordering by cost could evaluate them (or
compute the aliases
// in a lower projection) ahead of the conditions written before them.
+ // For the same reason, there is no split when a condition after the
first one held back
+ // above is rejected by `canPushAhead`, with only the cheap aliases
substituted (it can
+ // tell nothing about an expensive one such as a UDF): e.g. a division
over an expensive
+ // alias, or a cheap condition held back above, which needs no
expensive alias and so
+ // would go lowest. The filter then stays above the projection as
written, as without
+ // the split.
+ val cheapAliasMap = AttributeMap(aliasMap.filter { case (_, a) =>
!a.child.expensive })
+ val keepsWrittenOrder =
!SQLConf.get.legacyPushFilterAheadOfExpensiveConjunct &&
+ expensiveHeld.exists {
+ case (cond, _, _) => !canPushAhead(replaceAlias(cond,
cheapAliasMap))
+ }
Review Comment:
Done: `keepsWrittenOrder`, which builds the map, is a `lazy val` now, the
split config is checked first, and the map is only built when `expensiveHeld`
is non-empty. `evaluatedHere` checks `alias.child.expensive` before copying the
tree.
--
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]
