spark-test commented on code in PR #60060:
URL: https://github.com/apache/spark/pull/60060#discussion_r4471925692
##########
sql/core/src/test/scala/org/apache/spark/sql/collation/CollationTypePrecedenceSuite.scala:
##########
@@ -490,6 +490,63 @@ class CollationTypePrecedenceSuite extends
SharedSparkSession {
}
}
+ testFixedPointAndSinglePass("variant_get with a collated target type has
implicit strength") {
+ val tableName = "variant_get_tbl"
+ withTable(tableName) {
+ sql(s"CREATE TABLE $tableName (id INT, c STRING COLLATE UNICODE_CI, p
STRING, v VARIANT) " +
+ s"USING $dataSource")
+ sql(
+ s"""
+ |INSERT INTO $tableName SELECT id, 'OPEN', 'OPEN', parse_json(j)
FROM VALUES
+ | (1, '{"s": "Open", "a": ["Open"]}'),
+ | (2, '{"s": "open", "a": ["open"]}'),
+ | (3, '{"s": "closed", "a": ["closed"]}') AS t(id, j)
+ |""".stripMargin)
+
+ Seq("variant_get", "try_variant_get").foreach { fn =>
+ val lcase = s"$fn(v, '$$.s', 'STRING COLLATE UTF8_LCASE')"
+ val lcaseMap = s"$fn(v, '$$', 'MAP<STRING, STRING COLLATE
UTF8_LCASE>')"
+ val mixedStruct = s"$fn(v, '$$', 'STRUCT<s: STRING, t: STRING COLLATE
UTF8_LCASE>')"
+ checkAnswer(
+ sql(s"SELECT DISTINCT COLLATION($lcase || 'a') FROM $tableName"),
+ Row(UTF8_LCASE_COLLATION_NAME))
+
+ Seq(
+ s"$lcase = 'OPEN'",
+ s"$lcase IN ('OPEN', 'x')",
+ s"startswith($lcase, 'OP')",
+ s"array_contains($fn(v, '$$.a', 'ARRAY<STRING COLLATE UTF8_LCASE>'),
'OPEN')",
+ s"$fn(v, '$$', 'STRUCT<s: STRING COLLATE UTF8_LCASE>').s = 'OPEN'",
+ s"$lcaseMap['s'] = 'OPEN'",
+ s"element_at($lcaseMap, 's') = 'OPEN'").foreach { cond =>
+ checkAnswer(sql(s"SELECT id FROM $tableName WHERE $cond"),
Seq(Row(1), Row(2)))
+ }
+
+ // The UTF8_BINARY field of a partly collated struct stays UTF8_BINARY.
+ checkAnswer(sql(s"SELECT id FROM $tableName WHERE $mixedStruct.s =
'OPEN'"), Seq.empty)
+
+ checkAnswer(
+ sql(s"SELECT id FROM $tableName WHERE $lcase = 'Open' COLLATE
UTF8_BINARY"),
+ Seq(Row(1)))
+
+ // Like a column of the target type, it conflicts with other implicit
collations.
+ Seq(
+ s"$lcase = c",
+ s"$lcase = p",
+ s"$lcase IN (p, 'x')",
+ s"$fn(v, '$$.s', 'STRING COLLATE UTF8_BINARY') = c",
+ s"$mixedStruct.s = c").foreach { cond =>
+ assertIndeterminateCollation(sql(s"SELECT id FROM $tableName WHERE
$cond"))
+ }
+
+ // A plain STRING target keeps default strength.
Review Comment:
**Nit (P3):** This plain `STRING` case is the only check of where the new
rule stops applying, and every case above uses a UTF8_BINARY literal path
outside any default collation. Three regressions would pass the whole test:
- Replacing the recursive `hasNonDefaultStringCharOrVarcharType` check with
a top-level one, which is true for every array, map and struct, would make
`array_contains(variant_get(v, '$.a', 'ARRAY<STRING>'), c)` meet two implicit
collations instead of matching ids 1 and 2 under `c`'s UNICODE_CI.
- If the path argument's collation took part again, as it did before this
change, no case would notice, because a Default literal path never beats the
target. A collated path such as `variant_get(v, '$.s' COLLATE UNICODE, 'STRING
COLLATE UTF8_LCASE') = 'OPEN'` would catch it.
- No case runs in a view or table with a `DEFAULT COLLATION`, where an
explicit `STRING COLLATE UTF8_BINARY` target must win over the recolored
literals.
One case per boundary, under both analyzers, would pin them.
**Verification:**
- **Behavior:** An all-default ARRAY<STRING> target checked with
array_contains against a UNICODE_CI column returns the case-insensitive matches
under both analyzers.
- **Behavior:** A STRING COLLATE UTF8_LCASE target whose path argument
carries another collation compares in UTF8_LCASE under both analyzers.
- **Behavior:** Inside a view with DEFAULT COLLATION UTF8_LCASE, an explicit
STRING COLLATE UTF8_BINARY target compared with a literal matches
case-sensitively under both analyzers.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -399,6 +401,11 @@ object CollationTypeCoercion extends SQLConfHelper {
Some(addContextToStringType(cast.dataType, castStrength))
+ // An explicitly collated target type (even UTF8_BINARY), not the path,
decides the
+ // collation, like a column of that type.
+ case v: VariantGet if
DataTypeUtils.hasNonDefaultStringCharOrVarcharType(v.dataType) =>
Review Comment:
**Non-blocking (P2):** This arm keeps collated `variant_get` join keys
non-binary, so a join such as `variant_get(a.v, '$.k', 'STRING COLLATE
UTF8_LCASE') = variant_get(b.v, '$.k', 'STRING COLLATE UTF8_LCASE')` now
compares case-insensitively. Before this change both keys were cast to
`STRING`, so the join and any runtime filter on it agreed byte for byte.
`RewriteCollationJoin` wraps only attribute pairs in `CollationKey`, so these
keys reach `InjectRuntimeFilter` as they are. That rule skips non-binary-stable
keys only on its materialized-cache path (`InjectRuntimeFilter.scala:314-315`).
Its selective-predicate path still builds `BloomFilterAggregate(XxHash64(key))`
and probes with `BloomFilterMightContain(..., XxHash64(key))` (lines 78-93).
`XxHash64` hashes the raw bytes of a non-binary string unless
`spark.sql.legacy.collationAwareHashFunctions` is set, and that flag is off by
default.
So when the default-on runtime filter applies (an application-side scan of
at least 10GB, a selectively filtered creation side under 10MB, and a shuffle
on the application side), an application row with `'Open'` whose only match is
`'open'` is filtered out before the join. For inner, left semi, left outer and
right outer joins, whether the fix holds then depends on size statistics. A
left or right outer join null-extends the preserved row instead of matching it.
Other computed collated join keys already have the same gap on the base branch.
**Recommended change:** Make the runtime bloom filter rule skip a join key
whose type is not binary-stable on the selective-predicate path, as it already
does for materialized caches, and add runtime-filter join tests with collated
variant_get keys whose values differ only by case.
**Why this works:** A key that is not binary-stable can compare equal under
its collation while hashing differently. Refusing such keys before a creation
side is chosen means no bloom filter can reject a row that the join would
match, so the join's collation-aware comparison alone decides the result.
**Scope:** Tighten runtime bloom filter key eligibility in the optimizer and
cover collated join keys in runtime-filter tests.
**Compatibility:** Runtime filter injection for binary-stable join keys,
including the existing materialized-cache handling, is unchanged.
**Risks:** Runtime bloom filters are no longer injected for joins on
non-binary collated keys, including computed collated keys that received
unsound filters on the base branch, so those joins can read and shuffle more
rows; this is a plan-only change that can be rolled back, and collation-aware
hashing could restore the pruning later.
**Constraints:** Joins on binary-stable keys, including attribute pairs that
RewriteCollationJoin wraps in CollationKey, keep their current runtime filter
behavior.
**Exclusions:**
- Collation-aware hashing for runtime bloom filters, which would keep
pruning for collated keys, is left for a follow-up.
**Success:** No runtime bloom filter is injected for an equi-join key whose
type is not binary-stable. Joins on collated variant_get keys return every
case-variant match whether or not runtime bloom filters are enabled.
**Verification:**
- **Regression:** For inner, left semi (including one built from an IN
subquery), left outer and right outer joins on two collated variant_get keys
whose values differ only by case, with runtime bloom filters forced on through
a small application-side threshold, a selective creation side and broadcast
disabled, each query returns the case-insensitive matches, an outer join
null-extends no row that has such a match, and the optimized plan contains no
bloom filter.
- **Compatibility:** Joins on binary-stable keys, including collated
attribute pairs that are rewritten to CollationKey, still receive runtime bloom
filters as on the base branch.
--
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]
