Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,12 @@
package org.apache.spark.sql.catalyst.optimizer

import scala.collection.immutable.HashSet

import org.apache.spark.sql.catalyst.analysis.{CleanupAliases, EliminateSubQueries}
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.expressions.aggregate._
import org.apache.spark.sql.catalyst.plans.Inner
import org.apache.spark.sql.catalyst.plans.FullOuter
import org.apache.spark.sql.catalyst.plans.LeftOuter
import org.apache.spark.sql.catalyst.plans.RightOuter
import org.apache.spark.sql.catalyst.plans.LeftSemi
import org.apache.spark.sql.catalyst.planning.ExtractFiltersAndInnerJoins
import org.apache.spark.sql.catalyst.plans.{FullOuter, Inner, LeftOuter, LeftSemi, RightOuter}
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.rules._
import org.apache.spark.sql.types._
Expand All @@ -44,6 +42,7 @@ object DefaultOptimizer extends Optimizer {
// Operator push down
SetOperationPushDown,
SamplePushDown,
ReorderJoin,
PushPredicateThroughJoin,
PushPredicateThroughProject,
PushPredicateThroughGenerate,
Expand Down Expand Up @@ -711,6 +710,52 @@ object PushPredicateThroughAggregate extends Rule[LogicalPlan] with PredicateHel
}
}

/**
* Reorder the joins so that the bottom ones have at least one condition.
*/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add to this comment what makes this rule stable? It's not obvious from reading the code.

object ReorderJoin extends Rule[LogicalPlan] with PredicateHelper {

/**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove this comment if it is the same as the object comment or augment this with more detail.

Can you comment what the input arguments are? What is input? The least common ancestor of joins? Similar for conditions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updated

* Join a list of plans together and push down the conditions into them.
*
* The joined plan are picked from left to right, prefer those has at least one join condition.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Does this mean we generate a new identical tree each time this is run? Does this mess up the optimizer termination logic?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The transform will try to use original tree if a rule returns a identical tree, so don't need to have this optimization manually. @marmbrus is it right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be okay. We check reference equality first but then also check equals. As long as its not going to oscillate between plans it should terminate.

*
* @param input a list of LogicalPlans to join.
* @param conditions a list of condition for join.
*/
def createOrderedJoin(input: Seq[LogicalPlan], conditions: Seq[Expression]): LogicalPlan = {
assert(input.size >= 2)
if (input.size == 2) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

assert(input.size > 2)? then we don't need this if branch

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need a branch to terminate this recursive call anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah sorry I missed it

Join(input(0), input(1), Inner, conditions.reduceLeftOption(And))
} else {
val left = input.head
val rest = input.drop(1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Slightly more idiomatic to say val rest = input.tail.
or

val head :: tail = input.toList

// find out the first join that have at least one join condition
val conditionalJoin = rest.find { plan =>
val refs = left.outputSet ++ plan.outputSet
conditions.filterNot(_.references.subsetOf(left.outputSet))
.filterNot(_.references.subsetOf(plan.outputSet))
.exists(cond => cond.references.subsetOf(refs))
}
// pick the next one if no condition left
val right = conditionalJoin.getOrElse(rest.head)

val joinedRefs = left.outputSet ++ right.outputSet
val (joinConditions, others) = conditions.partition(_.references.subsetOf(joinedRefs))
val joined = Join(left, right, Inner, joinConditions.reduceLeftOption(And))

createOrderedJoin(Seq(joined) ++ rest.filterNot(_ eq right), others)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that this eq is safe, even in the presence of shared subtrees from self-joins (since the analyzer will rewrite the tree to avoid conflicting expression ids), but it might be slightly clearer to use partition above instead of find if thats not too much work.

}
}

def apply(plan: LogicalPlan): LogicalPlan = plan transform {
// TODO: support outer join

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

i would consider omitting this

case j @ ExtractFiltersAndInnerJoins(input, conditions) if input.size > 2 =>
assert(conditions.nonEmpty)
createOrderedJoin(input, conditions)
}
}

/**
* Pushes down [[Filter]] operators where the `condition` can be
* evaluated using only the attributes of the left or right side of a join. Other
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import org.apache.spark.Logging
import org.apache.spark.sql.catalyst.expressions._
import org.apache.spark.sql.catalyst.plans._
import org.apache.spark.sql.catalyst.plans.logical._
import org.apache.spark.sql.catalyst.trees.TreeNodeRef

/**
* A pattern that matches any number of project or filter operations on top of another relational
Expand Down Expand Up @@ -132,6 +131,44 @@ object ExtractEquiJoinKeys extends Logging with PredicateHelper {
}
}

/**
* A pattern that collects the filter and inner joins.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it much more work to extract all the filters? For example if there is a filter after the inner join of input and plan 1. We'd ideally use this for predicate progation as well.

For example

select * from t1 join t2 on t1.key = t2.key and t1.key = 5. If we collected all the filters, this could be used to infer t2.key = 5 and push that down to t2.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

*
* Filter
* |
* inner Join
* / \ ----> (Seq(plan0, plan1, plan2), conditions)
* Filter plan2
* |
* inner join
* / \
* plan0 plan1
*/
object ExtractFiltersAndInnerJoins extends PredicateHelper {

// flatten all inner joins, which are next to each other
def flattenJoin(plan: LogicalPlan): (Seq[LogicalPlan], Seq[Expression]) = plan match {
case Join(left, right, Inner, cond) =>
// only find the nested join on left, because we can only generate the plan like that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment is somewhat cryptic. Perhaps add below the picture something like, "This pattern currently only works for left-deep trees."

The reason for this limitation is that reorder currently doesn't know how to construct bushy plans right?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes

val (plans, conditions) = flattenJoin(left)
(plans ++ Seq(right), conditions ++ cond.toSeq)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be splitConjunctivePredicates(conditions) ++ cond.toSeq?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

conditions is already a list of Expression (splitted)


case Filter(filterCondition, j @ Join(left, right, Inner, joinCondition)) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Join(left, right, Inner, joinCondition) => Join(left, right, Inner, None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe just j @ Join(_, _, Inner, _)), the left, right and joinCondition are not used.

val (plans, conditions) = flattenJoin(j)
(plans, conditions ++ splitConjunctivePredicates(filterCondition))

case _ => (Seq(plan), Seq())
}

def unapply(plan: LogicalPlan): Option[(Seq[LogicalPlan], Seq[Expression])] = plan match {
case f @ Filter(filterCondition, j @ Join(_, _, Inner, _)) =>
Some(flattenJoin(f))
case j @ Join(_, _, Inner, _) =>
Some(flattenJoin(j))
case _ => None
}
}

/**
* A pattern that collects all adjacent unions and returns their children as a Seq.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class FilterPushdownSuite extends PlanTest {
CombineFilters,
PushPredicateThroughProject,
BooleanSimplification,
ReorderJoin,
PushPredicateThroughJoin,
PushPredicateThroughGenerate,
PushPredicateThroughAggregate,
Expand Down Expand Up @@ -548,6 +549,25 @@ class FilterPushdownSuite extends PlanTest {
comparePlans(optimized, analysis.EliminateSubQueries(correctAnswer))
}

test("joins: reorder inner joins") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is a big enough optimization that we might put it in its own suite.

val x = testRelation.subquery('x)
val y = testRelation1.subquery('y)
val z = testRelation.subquery('z)

val originalQuery = {
x.join(y).join(z)
.where(("x.b".attr === "z.b".attr) && ("y.d".attr === "z.a".attr))
}

val optimized = Optimize.execute(originalQuery.analyze)
val correctAnswer =
x.join(z, condition = Some("x.b".attr === "z.b".attr))
.join(y, condition = Some("y.d".attr === "z.a".attr))
.analyze

comparePlans(optimized, analysis.EliminateSubQueries(correctAnswer))
}

val testRelationWithArrayType = LocalRelation('a.int, 'b.int, 'c_arr.array(IntegerType))

test("generate: predicate referenced no generated column") {
Expand Down Expand Up @@ -750,4 +770,5 @@ class FilterPushdownSuite extends PlanTest {

comparePlans(optimized, correctAnswer)
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: spurious change

}