peter-toth commented on code in PR #59161:
URL: https://github.com/apache/spark/pull/59161#discussion_r4216667479
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/planmerging/PlanMerger.scala:
##########
@@ -1104,9 +1121,8 @@ class PlanMerger(
}
}
- newPlanFilter.foreach {
- case (f, true) => mergedExpressions += f
- case _ =>
+ newPlanFilter.foreach { case (f, _) =>
Review Comment:
**Finding 5.** With this change nothing reads the `Boolean` in
`newPlanFilter` any more. Every reader ignores it with `(f, _)`. That's this
line, line 1097, the `Aggregate` arm, the three `Filter` arms and the join arm.
Only the writers still set it (lines 554, 560, 613, 638, 702).
The `TryMergeResult` scaladoc (lines 312-315) still describes the old
contract. It says `false` means the attribute "was reused from an existing
alias already present in the merged plan". Finding 1 showed that doesn't hold
once the join arm folds the attribute into an `AND`.
Dropping the flag makes `newPlanFilter` an `Option[Attribute]`, like
`cachedPlanFilter`. The `isNew` remark on `cachedPlanFilter` (line 317) can go
with it.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/planmerging/PlanMerger.scala:
##########
@@ -690,19 +684,43 @@ class PlanMerger(
// rows are NULL-padded so f=NULL, causing FILTER
(WHERE f) to incorrectly
// exclude rows that should contribute to the
aggregate. Right-side
// attributes are also absent from semi/anti join
output.
- (leftNPFilter.isEmpty && leftCPFilter.isEmpty ||
+ (leftNPFilter.isEmpty && leftCPFilter.isEmpty ||
filterSafeForJoin(fromLeft = true, cp.joinType)) &&
(rightNPFilter.isEmpty && rightCPFilter.isEmpty ||
filterSafeForJoin(fromLeft = false, cp.joinType)) =>
val npMapping = leftNPMapping ++ rightNPMapping
val mappedNPCondition = np.condition.map(mapAttributes(_,
npMapping))
// Comparing the canonicalized form is required to ignore
different forms of the
// same expression and `AttributeReference.qualifier`s in
`cp.condition`.
- if (mappedNPCondition.map(_.canonicalized) ==
cp.condition.map(_.canonicalized)) {
- val npFilter = leftNPFilter.orElse(rightNPFilter)
- val cpFilter = leftCPFilter.orElse(rightCPFilter)
- Some(TryMergeResult(cp.withNewChildren(Seq(mergedLeft,
mergedRight)), npMapping,
- npFilter, cpFilter, dsv2Merged = leftDsv2Merged ||
rightDsv2Merged))
+ if (mappedNPCondition.map(_.canonicalized) ==
+ cp.condition.map(_.canonicalized)) {
+ val mergedJoin = cp.withNewChildren(Seq(mergedLeft,
mergedRight))
+ val (npFilter, npAlias) = (leftNPFilter, rightNPFilter)
match {
Review Comment:
**Finding 6.** With symmetric propagation on, both subplans can now filter
both children. Take `l.p = 1 AND r.p = 1` merged with `l.p = 2 AND r.p = 2`.
The merged join reads `l.p IN (1, 2)` on the left and `r.p IN (1, 2)` on the
right. It also builds the `(1, 2)` and `(2, 1)` pairs, which neither subplan
needs and both `FILTER`s drop.
Merges that keep their filters to one child never build such pairs. Joining
`L1 ∪ L2` with `R` gives exactly the pairs of `L1` with `R` plus those of `L2`
with `R`. The shape this PR targets is safe too, because there one subplan has
no filter at all.
How many extra pairs there are depends on the join:
- On an equi-join whose key doesn't depend on the filtered columns, there
can be as many unneeded pairs as needed ones.
- On a `Cross` join it's the product of the two widened sides.
- A side that was small enough to broadcast in each original subplan can
grow past the threshold.
The symmetric config doc only warns about IO pruning, so I'd name this cost
there too. Declining the shape outright would also block merges where one
subplan's filters imply the other's, like the `isnotnull` filters inferred for
a nullable join key.
--
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]
