Skip to content

[SPARK-29231][SQL] Constraints should be inferred from cast equality constraint #27252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wangyum wants to merge 10 commits into from
Closed

[SPARK-29231][SQL] Constraints should be inferred from cast equality constraint #27252

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
048a0ec
Constraints should be inferred from cast equality constraint
wangyum Jan 17, 2020
34c7001
Support filter at lower data type
wangyum Jan 19, 2020
7dcfe91
fix test error
wangyum Jan 20, 2020
2060190
Only support filter higher data type
wangyum Jan 31, 2020
420058b
IsNotNull should be constructed by `constructIsNotNullConstraints`.
wangyum Feb 4, 2020
8dcd076
Address comment
wangyum Feb 11, 2020
47eadf4
Address comment
wangyum Feb 12, 2020
d283028
constraints -> predicates
wangyum Feb 12, 2020
d055eba
Improve test coverage
wangyum Feb 12, 2020
4b14b3f
Improve test coverage
wangyum Feb 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions ...yst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/QueryPlanConstraints.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,17 @@ trait ConstraintHelper {
*/
def inferAdditionalConstraints(constraints: Set[Expression]): Set[Expression] = {
var inferredConstraints = Set.empty[Expression]
// IsNotNull should be constructed by `constructIsNotNullConstraints`.
val predicates = constraints.filterNot(_.isInstanceOf[IsNotNull])
constraints.foreach {
Copy link
Member

@maropu maropu Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot do it like predicates.foreach { here?

Copy link
Member Author

@wangyum wangyum Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We can do it.

case eq @ EqualTo(l: Attribute, r: Attribute) =>
val candidateConstraints = constraints - eq
val candidateConstraints = predicates - eq
inferredConstraints ++= replaceConstraints(candidateConstraints, l, r)
inferredConstraints ++= replaceConstraints(candidateConstraints, r, l)
case eq @ EqualTo(l @ Cast(_: Attribute, _, _), r: Attribute) =>
inferredConstraints ++= replaceConstraints(predicates - eq, r, l)
Copy link
Contributor

@cloud-fan cloud-fan Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to https://github.com/apache/spark/pull/27252/files#r378111623

If we have cast(a, dt) = b and b = 1, we can definitely infer cast(a, dt) = 1.
If we have cast(a, dt) = b and a = 1, seems we can also infer cast(1, dt) = b.

But I'm a bit unsure about how to do it. We may need a variant of replaceConstraints, which holds an expression builder "e => cast(e, dt) = b". It looks for attribute a, and replace it with cast(1, dt) = b

Copy link
Contributor

@peter-toth peter-toth Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to touch replaceConstraints at all. Please check #27518 that will do the trick because a = 1 will be "substituted" into cast(a, dt) = b as a new cast(1, dt) = b constraint.

Copy link
Contributor

@peter-toth peter-toth Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think both this PR and #27518 are beneficial. But I would use val originalLeft = testRelation1.where('a < 1).subquery('left) in test cases of this one instead of val originalLeft = testRelation1.where('a === 1).subquery('left) to avoid confusion.

Copy link
Member Author

@wangyum wangyum Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan This PR support cast(1, dt) = b before:
https://github.com/apache/spark/compare/048a0ecc65763c6feaa939938e2dec6f4040d939..7dcfe915087dbe274b470928600197745a645f5e

I removed it because:

  1. It may be broken the plan. This is how I handled it before.
  2. For cast(a, dt) = b, we support inferring many predicates, for example: a > 1, a < 1, a in (2, 3). I'm not sure if it's safe.

How about only supporting cast(a, dt) = 1 now?

@peter-toth I'd like to support these cases in #27518:

a < b && b < c infer a < c
a < b && b <= c infer a < c
a < b && b = c infer a < c
...

Copy link
Contributor

@peter-toth peter-toth Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangyum I see, but I think currently you are doing something very different in #27518 see details here: #27518 (comment)

I would suggest keeping your #27518 in its current form (but amending its title) and open a new one to address inequalities.

Copy link
Member

@maropu maropu Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about only supporting cast(a, dt) = 1 now?

+1 for supporting the limited case only in this pr. Since this part of optimization can affect many queries, I think we need exhaustive discussions and tests for supporting wider cases.

case eq @ EqualTo(l: Attribute, r @ Cast(_: Attribute, _, _)) =>
inferredConstraints ++= replaceConstraints(predicates - eq, l, r)
case _ => // No inference
}
inferredConstraints -- constraints
Expand All @@ -75,7 +81,7 @@ trait ConstraintHelper {
private def replaceConstraints(
constraints: Set[Expression],
source: Expression,
destination: Attribute): Set[Expression] = constraints.map(_ transform {
destination: Expression): Set[Expression] = constraints.map(_ transform {
case e: Expression if e.semanticEquals(source) => destination
})

Expand Down
37 changes: 35 additions & 2 deletions ...test/scala/org/apache/spark/sql/catalyst/optimizer/InferFiltersFromConstraintsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import org.apache.spark.sql.catalyst.plans._
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules._
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types.{IntegerType, LongType}

class InferFiltersFromConstraintsSuite extends PlanTest {

Expand All @@ -46,8 +47,8 @@ class InferFiltersFromConstraintsSuite extends PlanTest {
y: LogicalPlan,
expectedLeft: LogicalPlan,
expectedRight: LogicalPlan,
joinType: JoinType) = {
val condition = Some("x.a".attr === "y.a".attr)
joinType: JoinType,
condition: Option[Expression] = Some("x.a".attr === "y.a".attr)) = {
val originalQuery = x.join(y, joinType, condition).analyze
val correctAnswer = expectedLeft.join(expectedRight, joinType, condition).analyze
val optimized = Optimize.execute(originalQuery)
Expand Down Expand Up @@ -263,4 +264,36 @@ class InferFiltersFromConstraintsSuite extends PlanTest {
val y = testRelation.subquery('y)
testConstraintsAfterJoin(x, y, x.where(IsNotNull('a)), y, RightOuter)
}

test("Constraints should be inferred from cast equality constraint(filter higher data type)") {
val testRelation1 = LocalRelation('a.int)
val testRelation2 = LocalRelation('b.long)
val originalLeft = testRelation1.subquery('left)
val originalRight = testRelation2.where('b === 1L).subquery('right)

val left = testRelation1.where(IsNotNull('a) && 'a.cast(LongType) === 1L).subquery('left)
val right = testRelation2.where(IsNotNull('b) && 'b === 1L).subquery('right)

Seq(Some("left.a".attr.cast(LongType) === "right.b".attr),
Some("right.b".attr === "left.a".attr.cast(LongType))).foreach { condition =>
testConstraintsAfterJoin(originalLeft, originalRight, left, right, Inner, condition)
Copy link
Member

@maropu maropu Feb 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In terms of test coverage, its better to test both cases (left/right-side casts)? I have the same comment in the test below.

}
Copy link
Contributor

@cloud-fan cloud-fan Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also test cast(int) here. The key is: we should test both left side cast and right side cast, as @maropu said.

"left.a".attr.cast(LongType) === "right.b".attr and "right.b".attr === "left.a".attr.cast(LongType) only test left side cast (join left side, not EqualTo left side).

}

test("Constraints shouldn't be inferred from cast equality constraint(filter lower data type)") {
val testRelation1 = LocalRelation('a.int)
val testRelation2 = LocalRelation('b.long)
val originalLeft = testRelation1.where('a > 1).subquery('left)
val originalRight = testRelation2.where('b < 10).subquery('right)

val left = testRelation1.where(
IsNotNull('a) && 'a > 1 && 'a.cast(LongType) < Literal(10).cast(LongType)).subquery('left)
val right = testRelation2.where(
IsNotNull('b) && 'b < Literal(10).cast(LongType)).subquery('right)

Seq(Some("left.a".attr.cast(LongType) === "right.b".attr),
Copy link
Contributor

@peter-toth peter-toth Feb 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be wrong, but I find these test cases a bit confusing because left and right have equality filters. Eg. here 'a === 1 in left so actually it would be correct to infer 1.cast(LongType) === 'b for right. This PR doesn't do that obviously (#27518 will address that) but probably using inequalities ('a < 1) would be easier to follow.

Some("right.b".attr === "left.a".attr.cast(LongType))).foreach { condition =>
testConstraintsAfterJoin(originalLeft, originalRight, left, right, Inner, condition)
}
}
}