uros-b commented on code in PR #57705:
URL: https://github.com/apache/spark/pull/57705#discussion_r3700404370
##########
sql/core/src/test/scala/org/apache/spark/sql/InjectRuntimeFilterSuite.scala:
##########
@@ -519,6 +519,48 @@ class InjectRuntimeFilterSuite extends SharedSparkSession
}
}
+ test("SPARK-58486: runtime bloom filters use collation-aware hashing") {
Review Comment:
The regression test verifies plan structure only (two CollationAwareXxHash64
nodes in the optimised plan) but does not verify result correctness. The PR
description states the join returns 0 rows pre-fix and 5000 rows post-fix; this
claim should be pinned with an explicit checkAnswer(sql(query),
Seq(Row(5000L))). A test that only checks plan shape cannot catch a future
regression where the plan looks right but hashes still diverge; the result
check is the primary safety net for a silent-wrong-result fix.
##########
sql/core/src/test/scala/org/apache/spark/sql/InjectRuntimeFilterSuite.scala:
##########
@@ -519,6 +519,48 @@ class InjectRuntimeFilterSuite extends SharedSparkSession
}
}
+ test("SPARK-58486: runtime bloom filters use collation-aware hashing") {
+ withTable("runtime_filter_dim", "runtime_filter_fact") {
+ sql("""
+ |CREATE TABLE runtime_filter_dim(
+ | key STRING COLLATE UTF8_LCASE,
Review Comment:
No test covers non-UTF8_LCASE collations (UNICODE, UNICODE_CI). The
correctness argument extends to all non-binary collations (each uses
sortKeyFunction to normalise equivalent strings to the same collation key). A
single additional test with UNICODE_CI would prove the mechanism is not
LCASE-specific. Please take a look at other collation tests and follow the
common formats for collation testing.
##########
sql/core/src/test/scala/org/apache/spark/sql/InjectRuntimeFilterSuite.scala:
##########
@@ -519,6 +519,48 @@ class InjectRuntimeFilterSuite extends SharedSparkSession
}
}
+ test("SPARK-58486: runtime bloom filters use collation-aware hashing") {
+ withTable("runtime_filter_dim", "runtime_filter_fact") {
+ sql("""
+ |CREATE TABLE runtime_filter_dim(
+ | key STRING COLLATE UTF8_LCASE,
+ | category STRING) USING parquet
+ |""".stripMargin)
+ sql("INSERT INTO runtime_filter_dim VALUES ('ABC', 'x')")
+ sql("""
+ |CREATE TABLE runtime_filter_fact USING parquet AS
+ |SELECT concat('abc', CAST(id AS STRING)) COLLATE UTF8_LCASE AS key
+ |FROM range(5000)
+ |""".stripMargin)
+
+ val query =
+ """
+ |SELECT count(*)
+ |FROM runtime_filter_fact fact JOIN runtime_filter_dim dim
+ |ON substr(fact.key, 1, 3) = dim.key
+ |WHERE dim.category = 'x'
+ |""".stripMargin
+
+ withSQLConf(
+ SQLConf.RUNTIME_BLOOM_FILTER_ENABLED.key -> "true",
+ SQLConf.RUNTIME_BLOOM_FILTER_APPLICATION_SIDE_SCAN_SIZE_THRESHOLD.key
-> "1",
+ SQLConf.AUTO_BROADCASTJOIN_THRESHOLD.key -> "-1") {
+ assertRewroteWithBloomFilter(query)
+
+ val plan = sql(query).queryExecution.optimizedPlan
+ val bloomFilterHashes = plan.collectWithSubqueries {
+ case node => node.expressions.flatMap(_.collect {
+ case BloomFilterMightContain(_, hash) => hash
+ case AggregateExpression(
+ BloomFilterAggregate(hash, _, _, _, _), _, _, _, _) => hash
+ })
+ }.flatten
+ assert(bloomFilterHashes.size == 2)
+
assert(bloomFilterHashes.forall(_.isInstanceOf[CollationAwareXxHash64]))
+ }
+ }
+ }
+
Review Comment:
PTAL: sql/core/src/test/resources/tpcds-plan-stability/ (44 files, 201
occurrences). The PR changes the prettyName of every Bloom-filter hash
expression from xxhash64 to collation_aware_xxhash64 in query-plan text. The
TPC-DS plan-stability approved files hard-code the old name and CI "sql -
extended tests" is already failing with
TPCDSModifiedPlanStabilityWithStatsSuite,
TPCDSV1_4_PlanStabilityWithStatsSuite, and
TPCDSV2_7_PlanStabilityWithStatsSuite mismatches on q2, q10, q16, q24a/b, q32,
q37, q40, q59, q64, q69, q80, q80a, q82, q85, q92, q94, q95, q10a (confirmed in
CI annotations). The PR cannot merge without either (a) regenerating all
affected approved-plans-*/q*.sf100/simplified.txt and explain.txt files, or (b)
conditionally applying CollationAwareXxHash64 only when the join key's type
involves a non-binary-collated string (via
SchemaUtils.hasNonUTF8BinaryCollation(key.dataType)), leaving integer-keyed
Bloom filters (the TPC-DS case) on XxHash64. Option (b) is cleaner: it
avoids noisy plan diffs for all non-collated queries and sidesteps large
golden-file churn with no correctness trade-off.
--
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]
