peter-toth commented on code in PR #60054:
URL: https://github.com/apache/spark/pull/60054#discussion_r4484285500
##########
sql/core/src/test/scala/org/apache/spark/sql/collation/CollationSQLRegexpSuite.scala:
##########
@@ -69,19 +69,41 @@ class CollationSQLRegexpSuite
})
}
- test("Like simplification should work with collated strings") {
+ test("LIKE under UTF8_LCASE keeps matching semantics after optimization") {
+ // A literal-only LIKE is constant-folded before LikeSimplification and
never reaches
+ // the rule, so this test uses a column. The original LIKE matches dotless
i (U+0131)
+ // against pattern 'i' via Java regex case folding; the Contains rewrite
used ICU
+ // lowercase mapping and did not.
+ val tableName = "T_LCASE_DOTLESS_I"
+ withTable(tableName) {
+ sql(s"CREATE TABLE IF NOT EXISTS $tableName(c STRING COLLATE UTF8_LCASE)
using PARQUET")
+ sql(s"INSERT INTO $tableName(c) VALUES('ı')")
+ val query = sql(s"SELECT c LIKE '%i%' FROM $tableName")
Review Comment:
**Finding 2.** The description leads with `SELECT like(collate('ı',
'UTF8_LCASE'), '%i%')`. That query has only literals, so `ConstantFolding`
evaluates it before `LikeSimplification` runs, as the comment above says. It
returns `true` on the base as well. This column query is what actually changes.
I ran these with the base rule and on `744e7ab`, with `c STRING COLLATE
UTF8_LCASE`:
| `c` | predicate | base | head | `=` on both |
|---|---|---|---|---|
| `ı` | `c LIKE 'i'` | `false` | `true` | `c = 'i'` is `false` |
| `İ` | `c LIKE 'i'` | `false` | `true` | `c = 'i'` is `false` |
| `ſ` | `c LIKE 's'` | `false` | `true` | `c = 's'` is `false` |
`c LIKE '%i%'`, `c LIKE 'i%'` and `c LIKE ANY ('%i%', 'zz')` flip the same
way for `ı`. So simple-shape `LIKE` results change for such characters, and
`LIKE` no longer agrees with `=`, `startswith` and `contains` on them. Could
the description use the column example and say which results change? A line in
`docs/sql-migration-guide.md` would help too, saying that `LIKE` under
UTF8_LCASE follows Java's regex case folding and can disagree with `=`.
On the follow-up deferred at
[issuecomment-6622251925](https://github.com/apache/spark/pull/60054#issuecomment-6622251925):
matching the lowercase strings "would change existing LIKE results", but so
does this PR. The two directions change different shapes:
- **This PR** changes the shapes that used to be rewritten: prefix, suffix,
contains and equality. Those cover most `LIKE` predicates on columns.
- **The lowercase direction** would change the rest: patterns the rule never
rewrote, such as ones with `_` or `a%b%c`, and literal-only queries.
Only the lowercase direction keeps `c LIKE 'x'` equal to `c = 'x'` and keeps
this rewrite valid. If that is the long-term plan, this PR flips the common
shapes now and the follow-up flips them back.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/LikeSimplificationSuite.scala:
##########
@@ -390,32 +390,80 @@ class LikeSimplificationSuite extends PlanTest {
// scalastyle:off nonascii
test("SPARK-59063: LikeSimplification preserves LIKE semantics under
non-binary collation") {
- // Under UTF8_LCASE, StartsWith/EndsWith are collation-aware, so a single
code point
- // whose case-folded form equals the anchor satisfies BOTH
StartsWith(prefix) and
- // EndsWith(suffix). The Kelvin sign (U+212A) is one code point of three
UTF-8 bytes
- // that folds to 'k'. LIKE 'k%k' is false for it -- the pattern requires
two 'k's --
- // so the rewrite's length guard must reject it. A byte-length guard
(OctetLength >=
- // numBytes("k") + numBytes("k") = 2) is satisfied by the 3-byte Kelvin
sign and would
- // wrongly accept it; the code-point guard kept for non-binary collations
rejects it.
+ // LIKE under UTF8_LCASE matches via Java regex case folding, while the
predicates a
+ // rewrite would use (StartsWith/EndsWith plus a length guard) match via
ICU lowercase
+ // mapping. A single multibyte code point can satisfy both anchors and
clear the byte
+ // guard: the Kelvin sign (U+212A) is three UTF-8 bytes and lowercases to
'k', so LIKE
+ // 'k%k' is false for it (the pattern needs two 'k's) while the rewrite
Review Comment:
**Finding 3.** The base rule did not accept the Kelvin sign here. It kept
the code-point `Length` guard for non-binary collations (the branch this PR
removes), so `'k%k'` was rewritten but still rejected a single Kelvin sign.
Only a byte guard would accept it. Could the comment say that, or just say that
the test now checks `'k%k'` isn't rewritten?
--
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]
