Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/20444#discussion_r164937374
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ReplaceExceptWithFilter.scala
---
@@ -46,18 +46,27 @@ object ReplaceExceptWithFilter extends
Rule[LogicalPlan] {
}
plan.transform {
- case Except(left, right) if isEligible(left, right) =>
- Distinct(Filter(Not(transformCondition(left, skipProject(right))),
left))
+ case e @ Except(left, right) if isEligible(left, right) =>
+ val newCondition = transformCondition(left, skipProject(right))
+ newCondition.map { c =>
+ Distinct(Filter(Not(c), left))
+ }.getOrElse {
+ e
+ }
}
}
- private def transformCondition(left: LogicalPlan, right: LogicalPlan):
Expression = {
+ private def transformCondition(left: LogicalPlan, right: LogicalPlan):
Option[Expression] = {
val filterCondition =
InferFiltersFromConstraints(combineFilters(right)).asInstanceOf[Filter].condition
val attributeNameMap: Map[String, Attribute] = left.output.map(x =>
(x.name, x)).toMap
- filterCondition.transform { case a : AttributeReference =>
attributeNameMap(a.name) }
+ if (filterCondition.references.forall(r =>
attributeNameMap.contains(r.name))) {
+ Some(filterCondition.transform { case a: AttributeReference =>
attributeNameMap(a.name) })
--- End diff --
Actually it may still possibly to add the `Filter` on the child of left's
projection where it can be applied. But for now this fixing LGTM.
We may also need to update the doc of `ReplaceExceptWithFilter` to add this
constraint.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
