Mailing-List: contact reviews-help@spark.apache.org; run by ezmlm
Precedence: bulk
From: cloud-fan <git@git.apache.org>
To: reviews@spark.apache.org
Reply-To: reviews@spark.apache.org
References: <git-pr-19201-spark@git.apache.org>
In-Reply-To: <git-pr-19201-spark@git.apache.org>
Subject: [GitHub] spark pull request #19201: [SPARK-21979][SQL]Improve QueryPlanConstraints fr...
Content-Type: text/plain
Message-Id: <20170912165533.7E466F5724@git1-us-west.apache.org>
Date: Tue, 12 Sep 2017 16:55:33 +0000 (UTC)
archived-at: Tue, 12 Sep 2017 16:55:36 -0000

Github user cloud-fan commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19201#discussion_r138384197
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/QueryPlanConstraints.scala ---
    @@ -106,91 +106,48 @@ trait QueryPlanConstraints { self: LogicalPlan =>
        * Infers an additional set of constraints from a given set of equality constraints.
        * For e.g., if an operator has constraints of the form (`a = 5`, `a = b`), this returns an
        * additional constraint of the form `b = 5`.
    -   *
    -   * [SPARK-17733] We explicitly prevent producing recursive constraints of the form `a = f(a, b)`
    -   * as they are often useless and can lead to a non-converging set of constraints.
        */
       private def inferAdditionalConstraints(constraints: Set[Expression]): Set[Expression] = {
    -    val constraintClasses = generateEquivalentConstraintClasses(constraints)
    -
    +    val aliasedConstraints = eliminateAliasedExpressionInConstraints(constraints)
         var inferredConstraints = Set.empty[Expression]
    -    constraints.foreach {
    +    aliasedConstraints.foreach {
           case eq @ EqualTo(l: Attribute, r: Attribute) =>
    -        val candidateConstraints = constraints - eq
    -        inferredConstraints ++= candidateConstraints.map(_ transform {
    -          case a: Attribute if a.semanticEquals(l) &&
    -            !isRecursiveDeduction(r, constraintClasses) => r
    -        })
    -        inferredConstraints ++= candidateConstraints.map(_ transform {
    -          case a: Attribute if a.semanticEquals(r) &&
    -            !isRecursiveDeduction(l, constraintClasses) => l
    -        })
    +        val candidateConstraints = aliasedConstraints - eq
    +        inferredConstraints ++= replaceConstraints(candidateConstraints, l, r)
    +        inferredConstraints ++= replaceConstraints(candidateConstraints, r, l)
           case _ => // No inference
         }
         inferredConstraints -- constraints
       }
     
       /**
    -   * Generate a sequence of expression sets from constraints, where each set stores an equivalence
    -   * class of expressions. For example, Set(`a = b`, `b = c`, `e = f`) will generate the following
    -   * expression sets: (Set(a, b, c), Set(e, f)). This will be used to search all expressions equal
    -   * to an selected attribute.
    +   * Replace the aliased expression in [[Alias]] with the alias name if both exist in constraints.
    +   * Thus non-converging inference can be prevented.
    +   * E.g. `a = f(a, b)`,  `a = f(b, c) && c = g(a, b)`.
    +   * Also, the size of constraints is reduced without losing any information.
    +   * When the inferred filters are pushed down the operators that generate the alias,
    +   * the alias names used in filters are replaced by the aliased expressions.
        */
    -  private def generateEquivalentConstraintClasses(
    -      constraints: Set[Expression]): Seq[Set[Expression]] = {
    -    var constraintClasses = Seq.empty[Set[Expression]]
    -    constraints.foreach {
    -      case eq @ EqualTo(l: Attribute, r: Attribute) =>
    -        // Transform [[Alias]] to its child.
    -        val left = aliasMap.getOrElse(l, l)
    -        val right = aliasMap.getOrElse(r, r)
    -        // Get the expression set for an equivalence constraint class.
    -        val leftConstraintClass = getConstraintClass(left, constraintClasses)
    -        val rightConstraintClass = getConstraintClass(right, constraintClasses)
    -        if (leftConstraintClass.nonEmpty && rightConstraintClass.nonEmpty) {
    -          // Combine the two sets.
    -          constraintClasses = constraintClasses
    -            .diff(leftConstraintClass :: rightConstraintClass :: Nil) :+
    -            (leftConstraintClass ++ rightConstraintClass)
    -        } else if (leftConstraintClass.nonEmpty) { // && rightConstraintClass.isEmpty
    -          // Update equivalence class of `left` expression.
    -          constraintClasses = constraintClasses
    -            .diff(leftConstraintClass :: Nil) :+ (leftConstraintClass + right)
    -        } else if (rightConstraintClass.nonEmpty) { // && leftConstraintClass.isEmpty
    -          // Update equivalence class of `right` expression.
    -          constraintClasses = constraintClasses
    -            .diff(rightConstraintClass :: Nil) :+ (rightConstraintClass + left)
    -        } else { // leftConstraintClass.isEmpty && rightConstraintClass.isEmpty
    -          // Create new equivalence constraint class since neither expression presents
    -          // in any classes.
    -          constraintClasses = constraintClasses :+ Set(left, right)
    -        }
    -      case _ => // Skip
    +  private def eliminateAliasedExpressionInConstraints(constraints: Set[Expression])
    +    : Set[Expression] = {
    +    val attributesInEqualTo = constraints.flatMap {
    --- End diff --
    
    make it a set?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org