spark-test commented on code in PR #60060:
URL: https://github.com/apache/spark/pull/60060#discussion_r4488772195
##########
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:
Agreed that other computed collated join keys already have this gap on the
base branch. These keys did not: there, both variant_get keys were cast to
STRING before the join, so the join and the bloom filter compared the same
bytes. With this change the keys stay collated, so an injected runtime bloom
filter can drop exactly the case-variant matches this PR is meant to return,
and the result then depends on table statistics. That leaves the fix incomplete
for the joins it targets. Skipping keys that are not binary-stable on the
selective-predicate path, as the materialized-cache path already does, would
close it with a small change.
<!-- SPARK_DEV_REVIEW_REPLY
{"feedback_id":"inline:4471925682","thread_id":"inline:4471925682","verdict_sha256":"59061fb453faa1efe1d363432f1e0c8210bcaa166a1a94e30ae4fd2465106af8"}
-->
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -390,15 +392,23 @@ object CollationTypeCoercion extends SQLConfHelper {
case collate: Collate =>
Some(addContextToStringType(collate.dataType, Explicit))
+ // A cast from a string, or from VARIANT to an explicitly collated type
(even UTF8_BINARY),
+ // keeps that collation like a column of that type; `v:a::STRING COLLATE
X` is such a cast.
case cast: Cast =>
- val castStrength = if (hasStringType(cast.child.dataType)) {
+ val castStrength = if (hasStringType(cast.child.dataType) ||
Review Comment:
**Blocking (P1):** Giving a cast from VARIANT with an explicit collation
Implicit strength makes joins valid that the base branch rejected, and those
joins can now lose rows silently. Take `a JOIN b ON a.v:k::STRING COLLATE
UTF8_LCASE = b.v:k::STRING`. On the base branch both casts had Default strength
in different collations, so analysis failed with
`INDETERMINATE_COLLATION_IN_EXPRESSION`. Now the explicit cast wins, the other
key is retargeted to UTF8_LCASE, and the join matches case-insensitively on
expression keys that `RewriteCollationJoin` does not wrap in `CollationKey`.
When the default-on runtime bloom filter applies (an application side that
scans at least 10GB, a selectively filtered creation side under 10MB, and a
shuffle join), the selective-predicate path of `InjectRuntimeFilter` builds
`BloomFilterAggregate(XxHash64(key))`, which hashes the raw bytes of these
non-binary keys. A row of `a` with `'Open'` whose only match in `b` is `'open'`
fails `BloomFilterMightContain` and is removed before the join, with no error;
an outer join pruned on its null-supplying side null-extends the row instead.
The same holds when the other key is any `variant_get` call or `CAST(b.id AS
STRING)`. For collated `variant_get` keys the base branch already returned
case-sensitive matches, but these queries never ran before, so the change turns
an explicit error into a result whose completeness depends on table statistics.
**Recommended change:** Make InjectRuntimeFilter skip a join key whose type
is not binary-stable on the selective-predicate bloom filter path, as it
already does for materialized caches, and add runtime-filter join tests whose
keys differ only by case: collated variant_get keys, and casts from VARIANT to
an explicit collation joined to plain casts or variant_get calls.
**Why this works:** A key that is not binary-stable can compare equal under
its collation while hashing differently. If such keys are refused before a
creation side is chosen, no bloom filter can reject a row that the join would
match, and the join's collation-aware comparison alone decides the result.
**Scope:** Tighten runtime bloom filter key eligibility in the optimizer.
**Compatibility:** Runtime filter injection for binary-stable join keys is
unchanged, including the materialized-cache handling.
**Risks:** Runtime bloom filters are no longer injected for joins on
non-binary collated keys, including computed collated keys that receive unsound
filters on the base branch, so those joins can read and shuffle more rows. This
is a plan-only change and can be rolled back.
**Constraints:** Joins on binary-stable keys keep their current runtime
filter behavior, including collated attribute pairs that RewriteCollationJoin
wraps in CollationKey.
**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. Joins that
this change newly accepts in a non-binary collation, such as a cast from
VARIANT with an explicit collation joined to a plain cast from VARIANT, return
every match whether or not runtime bloom filters are enabled.
**Verification:**
- **Regression:** Force runtime bloom filters on, with a small
application-side threshold, a selective creation side and broadcast disabled.
Then 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 each return the case-insensitive matches. No outer join null-extends a row
that has such a match, and the optimized plan has no bloom filter on those keys.
- **Regression:** With the same settings, an inner join and a left semi join
on a.v:k::STRING COLLATE UTF8_LCASE = b.v:k::STRING, and an inner join on
a.v:k::STRING COLLATE UTF8_LCASE = variant_get(b.v, '$.k', 'STRING'), which
fail at analysis on the base branch, return every case-insensitive match, and
the optimized plan has no bloom filter on those keys.
- **Compatibility:** Joins on binary-stable keys still receive runtime bloom
filters as on the base branch, including collated attribute pairs that are
rewritten to CollationKey.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -390,15 +392,23 @@ object CollationTypeCoercion extends SQLConfHelper {
case collate: Collate =>
Some(addContextToStringType(collate.dataType, Explicit))
+ // A cast from a string, or from VARIANT to an explicitly collated type
(even UTF8_BINARY),
+ // keeps that collation like a column of that type; `v:a::STRING COLLATE
X` is such a cast.
case cast: Cast =>
- val castStrength = if (hasStringType(cast.child.dataType)) {
+ val castStrength = if (hasStringType(cast.child.dataType) ||
+ (cast.child.dataType.isInstanceOf[VariantType] &&
+
DataTypeUtils.hasNonDefaultStringCharOrVarcharType(cast.dataType))) {
Review Comment:
**Blocking (P1):** `hasNonDefaultStringCharOrVarcharType` treats a target as
explicitly collated whenever it is not the `StringType` companion object, but
by the time this arm runs the analyzer may already have replaced a plain
`STRING` target with a new instance. Under an object default collation,
`ApplyDefaultCollation` (and `DefaultCollationTypeCoercion` in the single-pass
analyzer) replaces the companion target of every user cast with
`StringType(<default collation>)`, so a plain cast from VARIANT now reads as
explicit and gets Implicit strength.
For example, with `t (id INT, p STRING, v VARIANT)`:
```sql
CREATE VIEW vw DEFAULT COLLATION UTF8_LCASE AS
SELECT id FROM t WHERE v:s::STRING = p;
```
On the base branch the cast keeps Default strength, `p` wins, and the view
works. Now the cast is Implicit UTF8_LCASE, it conflicts with the Implicit
UTF8_BINARY of `p`, and the statement fails with
`INDETERMINATE_COLLATION_IN_EXPRESSION`. The same happens in CTAS and RTAS,
temporary views and `ALTER VIEW AS`, and whenever a stored view or SQL function
with a default collation is analyzed again, so existing objects of this shape
start failing after an upgrade. `variant_get(v, '$.s', 'STRING') = p` in the
same view still works, because its target is never rewritten. Under `DEFAULT
COLLATION UTF8_BINARY` the fixed-point analyzer discards the value-equal
rewrite while the single-pass analyzer keeps the new instance, so the two
analyzers disagree on the same query. Coercion's own retargeting
(`cast.copy(dataType = ...)`) also produces non-companion types, so in a nested
merge a plain cast can flip to Implicit in a later pass. The new view test
compares `plain_cast` only with a recolor
ed literal, where Default and Implicit strength give the same answer, so it
does not catch this.
This extends existing discussions
[1](https://github.com/apache/spark/pull/60060#issuecomment-6664340600),
[2](https://github.com/apache/spark/pull/60060#pullrequestreview-5779474253).
See **Shared repair plan 1** in the review body.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CollationTypeCoercion.scala:
##########
@@ -390,15 +392,23 @@ object CollationTypeCoercion extends SQLConfHelper {
case collate: Collate =>
Some(addContextToStringType(collate.dataType, Explicit))
+ // A cast from a string, or from VARIANT to an explicitly collated type
(even UTF8_BINARY),
+ // keeps that collation like a column of that type; `v:a::STRING COLLATE
X` is such a cast.
case cast: Cast =>
- val castStrength = if (hasStringType(cast.child.dataType)) {
+ val castStrength = if (hasStringType(cast.child.dataType) ||
+ (cast.child.dataType.isInstanceOf[VariantType] &&
Review Comment:
**Non-blocking (P2):** This checks only for a top-level `VariantType`
source, while `hasStringType` just above checks the source recursively. A cast
whose source nests VARIANT inside an ARRAY or STRUCT, with no string in it,
therefore keeps Default strength even when its target is explicitly collated.
For example, `CAST(variant_get(v, '$.a', 'ARRAY<VARIANT>') AS ARRAY<STRING
COLLATE UTF8_LCASE>)[0] = 'open'` still fails with
`INDETERMINATE_COLLATION_IN_EXPRESSION`, and against a plain `STRING` column
the column's collation silently wins. That is the failure this PR fixes for
`v:a::ARRAY<STRING COLLATE UTF8_LCASE>`. The same applies to an
`ARRAY<VARIANT>` or `STRUCT<x: VARIANT>` column cast to a collated type. A
`MAP<STRING, VARIANT>` source is already Implicit through its string key, so
whether the declared collation applies now depends on how the extraction is
spelled.
**Verification:**
- **Regression:** Under both analyzers, CAST(variant_get(v, '$.a',
'ARRAY<VARIANT>') AS ARRAY<STRING COLLATE UTF8_LCASE>) matches
case-insensitively when compared with a literal through element access or
array_contains, and fails with INDETERMINATE_COLLATION_IN_EXPRESSION when
compared with a plain STRING column, as the direct VARIANT cast does. A
STRUCT<x: VARIANT> source cast to STRUCT<x: STRING COLLATE UTF8_LCASE> behaves
the same.
- **Compatibility:** Container casts of VARIANT values to plain STRING
targets keep Default strength, and casts whose source contains a string keep
Implicit strength.
--
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]
