peter-toth commented on code in PR #59916:
URL: https://github.com/apache/spark/pull/59916#discussion_r4472716485


##########
sql/core/src/test/scala/org/apache/spark/sql/InjectRuntimeFilterSuite.scala:
##########
@@ -1034,19 +1088,566 @@ class InjectRuntimeFilterSuite extends 
SharedSparkSession
 
   test("Runtime bloom filter join: do not add bloom filter if dpp filter 
exists " +
     "on the same column") {
-    
withSQLConf(SQLConf.RUNTIME_BLOOM_FILTER_APPLICATION_SIDE_SCAN_SIZE_THRESHOLD.key
 -> "3000",
-      SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "2000") {
-      assertDidNotRewriteWithBloomFilter("select * from bf5part join bf2 on " +
-        "bf5part.f5 = bf2.c2 where bf2.a2 = 62")
+    withDppTestConf(SQLConf.DYNAMIC_PARTITION_PRUNING_REUSE_BROADCAST_ONLY.key 
-> "false") {
+      val query = "select * from bf5part join bf2 on bf5part.f5 = bf2.c2 where 
bf2.a2 = 62"
+      assertDidNotRewriteWithBloomFilter(query)
+      assert(dynamicPruningFilters(query) == Seq(classOf[SubqueryExec]))
     }
   }
 
   test("Runtime bloom filter join: add bloom filter if dpp filter exists on " +
     "a different column") {
-    
withSQLConf(SQLConf.RUNTIME_BLOOM_FILTER_APPLICATION_SIDE_SCAN_SIZE_THRESHOLD.key
 -> "3000",
-      SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "2000") {
-      assertRewroteWithBloomFilter("select * from bf5part join bf2 on " +
-        "bf5part.c5 = bf2.c2 and bf5part.f5 = bf2.f2 where bf2.a2 = 62")
+    withDppTestConf(SQLConf.DYNAMIC_PARTITION_PRUNING_REUSE_BROADCAST_ONLY.key 
-> "false") {
+      val query = "select * from bf5part join bf2 on " +
+        "bf5part.c5 = bf2.c2 and bf5part.f5 = bf2.f2 where bf2.a2 = 62"
+      assertRewroteWithBloomFilter(query)
+      assert(dynamicPruningFilters(query) == Seq(classOf[SubqueryExec]))
+    }
+  }
+
+  test("SPARK-60702: add bloom filter if the dpp filter needs a broadcast that 
the shuffle " +
+    "join lacks") {
+    // `bf2.a2 = 36` keeps a row of `bf2` that joins, so the bloom filter is 
not empty when it meets
+    // the partition values of `bf5part`.
+    Seq(
+      "select * from bf5part join bf2 on bf5part.f5 = bf2.c2 where bf2.a2 = 
36",
+      "select * from bf2 join bf5part on bf2.c2 = bf5part.f5 where bf2.a2 = 
36").foreach { query =>
+      withDppTestConf() {
+        assertRewroteWithBloomFilterForDroppedDpp(query)
+        assert(sql(query).collect().nonEmpty)
+      }
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter if the dpp filter reuses the 
broadcast of " +
+    "the join") {
+    // The aggregate shuffles below the broadcast join, so the join may get a 
bloom filter. The
+    // join broadcasts `bf2` by a hint, with the partitioned table on either 
side, or by its size.
+    // Without DPP, or without exchange reuse to give the DPP filter the 
broadcast, the join gets
+    // the bloom filter.
+    Seq(
+      "select /*+ broadcast(bf2) */ * from (select f5, sum(a5) from bf5part 
group by f5) t " +
+        "join bf2 on t.f5 = bf2.c2 where bf2.a2 = 62" -> "2000",
+      "select /*+ broadcast(bf2) */ * from bf2 join (select f5, sum(a5) from 
bf5part " +
+        "group by f5) t on bf2.c2 = t.f5 where bf2.a2 = 62" -> "2000",
+      "select * from (select f5, sum(a5) from bf5part group by f5) t " +
+        "join bf2 on t.f5 = bf2.c2 where bf2.a2 = 62" -> "5000"
+    ).foreach { case (query, threshold) =>
+      withDppTestConf(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> threshold) {
+        withSQLConf(SQLConf.DYNAMIC_PARTITION_PRUNING_ENABLED.key -> "false") {
+          assertRewroteWithBloomFilter(query)
+        }
+        withSQLConf(SQLConf.EXCHANGE_REUSE_ENABLED.key -> "false") {
+          assertRewroteWithBloomFilterForDroppedDpp(query)
+        }
+        assertDidNotRewriteWithBloomFilter(query)
+        assert(dynamicPruningFilters(query) == 
Seq(classOf[SubqueryBroadcastExec]))
+      }
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter if another join broadcasts the 
build plan of " +
+    "the dpp filter") {
+    // The first join is a shuffle join, and its DPP filter reuses the 
broadcast of the second.
+    val query = "select p.f5, p.a5, d.* from bf5part p join bf2 d on p.f5 = 
d.c2 " +
+      "where d.a2 = 62 union all select /*+ broadcast(d) */ t.f5, t.s, d.* 
from " +
+      "(select f5, sum(a5) s from bf5part group by f5) t join bf2 d on t.f5 = 
d.c2 " +
+      "where d.a2 = 62"
+    withDppTestConf() {
+      // Without DPP, both `p.f5` and `t.f5` get a bloom filter.
+      assertBloomFilterWithoutDpp(query, 2)
+      assertDidNotRewriteWithBloomFilter(query)
+      // One of the DPP filters can read the broadcast through a reused 
subquery.
+      val reuses = Set[Any](classOf[SubqueryBroadcastExec], 
classOf[ReusedSubqueryExec])
+      val filters = dynamicPruningFilters(query)
+      assert(filters.size == 2 && filters.forall(reuses.contains), filters)
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter that takes a broadcast from a dpp 
filter") {
+    // The DPP filter on `q.f5` cannot reuse a broadcast, so it does not keep 
the bloom filter off
+    // its key. That bloom filter would go into `r`, the broadcast that the 
DPP filter on `p.f5`
+    // reuses. `q` is on either side of its join.
+    Seq(
+      "select /*+ broadcast(r) */ * from bf5part p join (select q.f5 rf, q.a5 
ra from " +
+        "bf5filtered q join bf2 on q.f5 = bf2.c2 where bf2.a2 = 62) r on p.f5 
= r.rf",
+      "select /*+ broadcast(r) */ * from bf5part p join (select q.f5 rf, q.a5 
ra from " +
+        "bf2 join bf5filtered q on bf2.c2 = q.f5 where bf2.a2 = 62) r on p.f5 
= r.rf"
+    ).foreach { query =>
+      withDppTestConf(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "500",
+          SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false") {
+        withSQLConf(SQLConf.DYNAMIC_PARTITION_PRUNING_ENABLED.key -> "false") {
+          assertRewroteWithBloomFilter(query)
+        }
+        assertDidNotRewriteWithBloomFilter(query)
+        
assert(dynamicPruningFilters(query).contains(classOf[SubqueryBroadcastExec]))
+      }
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter that takes a broadcast from a dpp 
filter in " +
+    "another union branch") {
+    // The DPP filter on `u.k` cannot reuse a broadcast, so it does not keep 
the bloom filter off
+    // its key. Pushdown would move that bloom filter into both branches, and 
so into `d`, the
+    // broadcast that the DPP filter on `q.f5` reuses. Over the cache of `d`, 
cleanup drops the
+    // copy of the DPP filter on `u.k`, but not the bloom filter.
+    val query = "select * from (select p.f5 k, p.a5 v from bf5part p union all 
select " +
+      "/*+ broadcast(d) */ d.c2 k, q.a5 v from bf5part q join bf2c d on q.f5 = 
d.c2 " +
+      "where d.a2 = 62) u join bf3 on u.k = bf3.c3 where bf3.a3 = 62"
+    withTempView("bf2c") {
+      withCache("bf2c") {
+        spark.table("bf2").createOrReplaceTempView("bf2c")
+        spark.catalog.cacheTable("bf2c")
+        withDppTestConf(SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false") {
+          // Without DPP, pushdown puts a bloom filter into each branch.
+          assertBloomFilterWithoutDpp(query, 2)
+          assertDidNotRewriteWithBloomFilter(query)
+          
assert(dynamicPruningFilters(query).contains(classOf[SubqueryBroadcastExec]))
+        }
+      }
+    }
+  }
+
+  test("SPARK-60702: keep the bloom filter off a key whose dpp filter reuses a 
broadcast side " +
+    "that has a dpp filter of its own") {
+    // The DPP filter on `q.f5` sits on top of `q` when the rule runs, and 
cleanup drops it later
+    // because of `q.f5 = 51`. Then the DPP filter on `t.f5` reuses the 
broadcast of `q`, so `t.f5`
+    // gets no bloom filter. `t.c5` gets one, since the DPP filter on its key 
is gone. `q` is on
+    // either side of the join.
+    Seq(
+      "select /*+ broadcast(q) */ * from (select f5, c5, sum(a5) s from 
bf5part " +
+        "where b5 > 0 group by f5, c5) t join bf5filtered q on t.f5 = q.c5 and 
t.c5 = q.f5 " +
+        "where q.a5 > 50 and q.f5 = 51",
+      "select /*+ broadcast(q) */ * from bf5filtered q join (select f5, c5, 
sum(a5) s from " +
+        "bf5part where b5 > 0 group by f5, c5) t on q.c5 = t.f5 and q.f5 = 
t.c5 " +
+        "where q.a5 > 50 and q.f5 = 51"
+    ).foreach { query =>
+      withDppTestConf() {
+        assertRewroteWithBloomFilter(query)
+        assert(dynamicPruningFilters(query) == 
Seq(classOf[SubqueryBroadcastExec]))
+      }
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter that takes a broadcast from a dpp 
filter that is " +
+    "not only in broadcast") {
+    // With `reuseBroadcastOnly` off, the DPP filter on `p.f5` has a benefit, 
so it can run as its
+    // own subquery, but physical planning reuses the broadcast of `r` first. 
The DPP filter on
+    // `q.f5` has none, so it is only in broadcast and cannot reuse a 
broadcast. A bloom filter on
+    // `q.f5` would go into `r`.
+    withTable("bf5big") {
+      sql("select a5 + id * 100 as a5, b5, c5, d5, e5, f5 from bf5part cross 
join range(400)")
+        .write.partitionBy("f5").saveAsTable("bf5big")
+      val query = "select /*+ broadcast(r) */ * from bf5big p join (select 
q.f5 rf, q.a5 ra " +
+        "from bf5filtered q join bf5part d on q.f5 = d.c5 where d.a5 = 62) r 
on p.f5 = r.rf"
+      withDppTestConf(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "500",
+          SQLConf.DYNAMIC_PARTITION_PRUNING_REUSE_BROADCAST_ONLY.key -> 
"false",
+          SQLConf.DYNAMIC_PARTITION_PRUNING_FALLBACK_FILTER_RATIO.key -> "1.0",
+          SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> "false") {
+        assertBloomFilterWithoutDpp(query)
+        assertDidNotRewriteWithBloomFilter(query)
+        
assert(dynamicPruningFilters(query).contains(classOf[SubqueryBroadcastExec]))
+      }
+    }
+  }
+
+  test("SPARK-60702: keep the bloom filter of a key without a dpp filter") {
+    // Pushdown moves the bloom filter on `s.k` into `x`, the broadcast that 
the DPP filter on
+    // `p.f5` reuses. The key has no DPP filter, so the rule adds the bloom 
filter all the same.
+    val query = "select * from (select x.c2 k, count(*) from bf5part p join 
(select c2 from bf2 " +
+      "where a2 = 62) x on p.f5 = x.c2 group by x.c2) s join bf3 on s.k = 
bf3.c3 " +
+      "where bf3.a3 = 62"
+    withDppTestConf() {
+      assertRewroteWithBloomFilter(query)
+    }
+  }
+
+  test("SPARK-60702: keep the dropped dpp filter of the key out of the bloom 
filter's creation " +
+    "side") {
+    // `q` is too small to take the bloom filter, so `b` takes it, and `q` is 
its creation side.
+    // The DPP filter on `q.f5` stays out of the bloom filter's subquery. `q` 
is on either side of
+    // the join.
+    withTable("bigapp") {
+      sql("select a5 + id * 100 as k, b5 from bf5part cross join range(400)")
+        .write.saveAsTable("bigapp")
+      Seq(
+        "select * from bf5filtered q join bigapp b on q.f5 = b.k where q.a5 > 
50 and b.b5 = 1",
+        "select * from bigapp b join bf5filtered q on b.k = q.f5 where q.a5 > 
50 and b.b5 = 1"
+      ).foreach { query =>
+        withDppTestConf(
+            
SQLConf.RUNTIME_BLOOM_FILTER_APPLICATION_SIDE_SCAN_SIZE_THRESHOLD.key -> 
"20000") {
+          assertRewroteWithBloomFilterForDroppedDpp(query)
+          val bloomFilterPlans = 
sql(query).queryExecution.optimizedPlan.collect {
+            case Filter(condition, _) => condition.collect {
+              case subquery: ScalarSubquery => subquery.plan
+            }
+          }.flatten
+          assert(bloomFilterPlans.size == 1)
+          val creationSide = bloomFilterPlans.head
+          assert(creationSide.collectLeaves().exists {
+            case LogicalRelationWithTable(_, Some(table)) =>
+              table.identifier.table == "bf5filtered"
+            case _ => false
+          }, creationSide)
+          assert(!creationSide.exists {
+            case Filter(_: DynamicPruningSubquery, _) => true
+            case _ => false
+          }, creationSide)
+        }
+      }
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter if physical planning keeps the 
dpp filter over the " +
+    "build plan of a null-aware anti join") {
+    // The join is a shuffle join. The `not in` subquery broadcasts the build 
plan of its DPP
+    // filter. Without AQE, physical planning keeps the filter, though it 
builds its own
+    // broadcast. AQE plans the filter as `true`, so there the key gets the 
bloom filter.
+    val query = "select * from bf5part p join (select c2 from bf2 where a2 = 
62 and c2 is not " +
+      "null) d on p.f5 = d.c2 where p.a5 not in (select c2 from bf2 where a2 = 
62 and c2 is not " +
+      "null)"
+    withDppTestConf(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "500",
+        SQLConf.NULL_AWARE_ANTI_JOIN_BROADCAST_THRESHOLD.key -> "-1") {
+      withSQLConf(SQLConf.DYNAMIC_PARTITION_PRUNING_ENABLED.key -> "false") {
+        assertRewroteWithBloomFilter(query)
+      }
+      if (conf.adaptiveExecutionEnabled) {
+        assertRewroteWithBloomFilterForDroppedDpp(query)
+      } else {
+        assertDidNotRewriteWithBloomFilter(query)
+        assert(dynamicPruningFilters(query) == 
Seq(classOf[SubqueryBroadcastExec]))
+      }
+    }
+  }
+
+  test("SPARK-60702: add bloom filter if the join type rules out the broadcast 
the dpp filter " +
+    "needs") {
+    // `bf2` is small enough to broadcast, but a right outer join cannot 
broadcast its right side,
+    // so the DPP filter does not count. The aggregate shuffles below the 
join, so the join can get
+    // the bloom filter.
+    val query = "select * from (select f5, sum(a5) s from bf5part group by f5) 
t " +
+      "right join bf2 on t.f5 = bf2.c2 where bf2.a2 = 62"
+    withDppTestConf(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "5000") {
+      assertRewroteWithBloomFilterForDroppedDpp(query)
+    }
+  }
+
+  test("SPARK-60702: add bloom filter if a shuffle hash hint rules out the 
broadcast the dpp " +
+    "filter needs") {
+    // `bf2` is small enough to broadcast, but the hint asks for a shuffle 
hash join. The aggregate
+    // shuffles below the join, so the join can get the bloom filter.
+    val query = "select /*+ shuffle_hash(bf2) */ * from (select f5, sum(a5) s 
from bf5part " +
+      "group by f5) t join bf2 on t.f5 = bf2.c2 where bf2.a2 = 62"
+    withDppTestConf(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "5000") {
+      assertRewroteWithBloomFilterForDroppedDpp(query)
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter if the dpp filter has a 
non-deterministic build " +
+    "plan") {
+    // The build plan of the DPP filter would be the creation side of the 
bloom filter. A bloom
+    // filter would evaluate it again, apart from the join, and could drop 
rows (SPARK-60808).
+    // Cleanup drops such a DPP filter.
+    val query = "select * from bf5part join (select * from bf2 where a2 = 36 
and " +
+      "cast(reflect('java.lang.Math', 'random') as double) < 2.0) d on 
bf5part.f5 = d.c2"
+    withDppTestConf() {
+      assertBloomFilterWithoutDpp(query)
+      assertDidNotRewriteWithBloomFilter(query)
+      assert(dynamicPruningFilters(query).isEmpty)
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter whose creation side is a 
non-deterministic pruned " +
+    "side") {
+    // The DPP filter on `d.k` has the deterministic build plan `f`, so it is 
dropped. `d` is too
+    // small to take a bloom filter, so `f` would take one, built from `d`, 
which is not
+    // deterministic (SPARK-60808).
+    withTable("ndd", "ndf") {
+      spark.range(40).selectExpr("id as k", "id as 
w").write.partitionBy("k").saveAsTable("ndd")
+      spark.range(40000).selectExpr("id % 40 as k", "id as 
v").write.saveAsTable("ndf")
+      sql("analyze table ndd compute statistics")
+      sql("analyze table ndf compute statistics")
+      val nddSize = 
spark.table("ndd").queryExecution.optimizedPlan.stats.sizeInBytes
+      val query = "select count(*) from (select k from ndd " +
+        "where cast(reflect('java.lang.Math', 'random') as double) < 0.5) d " +
+        "join ndf f on d.k = f.k where f.v >= 0"
+      withDppTestConf(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "-1",
+          
SQLConf.RUNTIME_BLOOM_FILTER_APPLICATION_SIDE_SCAN_SIZE_THRESHOLD.key ->
+            (nddSize + 1).toString) {
+        assert(getNumBloomFilters(sql(query).queryExecution.optimizedPlan) == 
0)
+        withSQLConf(SQLConf.DYNAMIC_PARTITION_PRUNING_ENABLED.key -> "false") {
+          // Without DPP, the key has no DPP filter, and the bloom filter 
comes as on master.
+          assert(getNumBloomFilters(sql(query).queryExecution.optimizedPlan) 
== 1)
+        }
+      }
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter if the key of the dropped dpp 
filter can fail") {
+    // On a partition column, the bloom filter would become a partition 
filter, which evaluates
+    // the key on every partition before the data filters (SPARK-60809). An 
alias hides the key only
+    // at the join, since pushdown rewrites the bloom filter through it. And 
`coalesce` cannot be
+    // null, so no inferred `isnotnull` evaluates the key on every partition 
first.
+    // Through a union, every branch counts, and without DPP each gets a bloom 
filter. A narrowing
+    // cast can fail too.
+    Seq(
+      "select * from bf5part join bf2 on bf5part.f5 + 1 = bf2.c2 where bf2.a2 
= 36" -> 1,
+      "select * from (select coalesce(f5 + 1, 0) as k, a5 from bf5part) p " +
+        "join bf2 on p.k = bf2.c2 where bf2.a2 = 36" -> 1,
+      "select * from (select f5 k, a5 v from bf5part union all " +
+        "select coalesce(f5 + 1, 0) k, a5 v from bf5filtered) u " +
+        "join bf2 on u.k = bf2.c2 where bf2.a2 = 36" -> 2,
+      "select * from bf5part join bf2 on cast(bf5part.f5 as tinyint) = bf2.c2 
" +
+        "where bf2.a2 = 36" -> 1
+    ).foreach { case (query, numBloomFilters) =>
+      withDppTestConf() {
+        assertBloomFilterWithoutDpp(query, numBloomFilters)
+        assertDidNotRewriteWithBloomFilter(query)
+        val filters = dynamicPruningFilters(query)
+        assert(filters.nonEmpty && filters.forall(_ == Literal.TrueLiteral), 
filters)
+      }
+    }
+  }
+
+  test("SPARK-60702: do not add bloom filter in an in subquery whose dpp 
filter reuses a " +
+    "broadcast outside it") {
+    // The optimizer runs on the subquery on its own, where the broadcast of 
`x` is not visible.
+    // The DPP filter on `p.f5` reuses it once the subquery is joined with the 
rest of the query.
+    val query = "select /*+ broadcast(x) */ * from bf3 join (select c2 from 
bf2 where a2 = 62) x " +
+      "on bf3.c3 = x.c2 where bf3.a3 in (select p.a5 from bf5part p join 
(select c2 from bf2 " +
+      "where a2 = 62) d on p.f5 = d.c2)"
+    def keysOnF5(query: String): Seq[Expression] =
+      bloomFilterKeys(query).filter(_.references.exists(_.name == "f5"))
+    withDppTestConf(SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "500") {
+      assert(keysOnF5(query).isEmpty)
+      assert(dynamicPruningFilters(query) == 
Seq(classOf[SubqueryBroadcastExec]))
+      // Without the hint, nothing broadcasts `x`, and `p.f5` gets the bloom 
filter.
+      val withoutHint = query.replace("/*+ broadcast(x) */ ", "")
+      assert(keysOnF5(withoutHint).nonEmpty)
+      assert(dynamicPruningFilters(withoutHint) == Seq(Literal.TrueLiteral))
+    }
+  }
+
+  test("SPARK-60702: add bloom filter for a key with a dropped dpp filter 
under a numeric " +
+    "up-cast") {
+    // Type coercion casts `f5` to `bigint`, which cannot fail.
+    val query = "select * from bf5part join (select cast(c2 as bigint) k, a2 
from bf2) d " +
+      "on bf5part.f5 = d.k where d.a2 = 36"
+    withDppTestConf() {
+      assertRewroteWithBloomFilterForDroppedDpp(query)
+    }
+  }
+
+  test("SPARK-60702: add bloom filter for a key with a dropped dpp filter of a 
floating point " +

Review Comment:
   I kept them separate, thanks.
   



-- 
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]

Reply via email to