shivadarshan-devadiga opened a new pull request, #58553:
URL: https://github.com/apache/spark/pull/58553
### What changes were proposed in this pull request?
This PR fixes five bugs in the CHAR type comparison rewrite path.
1. **`ApplyCharTypePaddingHelper` — IN list elements are misaligned with
their lengths.**
In the `In` branch, `literalCharLengths` was computed from the *non-null*
subset of the
list but then `zip`ped against the *original* list. Since `zip` truncates
to the shorter
sequence, a NULL element ahead of real literals shifted every later
length by one and
dropped trailing literals entirely. Lengths are now computed per element
as `Option[Int]`
so each stays paired with its own element, and NULL elements are left
untouched in place
(they can never match, so they need no padding).
Keeping the original element also fixes a second symptom: the old code
rebuilt NULLs as
`Literal.create(null, StringType)` with the *default* collation, while
`createStringRPad`
gives padded elements the column's collation. On a collated CHAR column
that mixture made
`In.checkInputDataTypes` fail, so the query did not return a wrong answer
— it did not run
at all.
2. **`TypeCoercionHelper.InTypeCoercion` — a redundant `Cast` hides the CHAR
column.**
When any list element's type differed from the value's, the rule cast
*every* child
unconditionally, including children already of the common type. An
untyped `NULL` in the
list therefore wrapped the value in `cast(c as string)`, and
`ApplyCharTypePadding`'s
`AttrOrOuterRef` extractor no longer matched it, so no padding was
applied at all. Note
`SimplifyCasts` deliberately preserves `Cast(charAttr, StringType)`, so
this cast survived
the whole optimizer rather than being a passing analysis-time artifact.
Now uses the
existing `castIfNotSameType` helper, and the guard moves to
`!haveSameType(...)` so that
guard and action agree — matching the `CreateArray` / `Concat` /
`MapConcat` / `Coalesce`
cases, which already pair `haveSameType` with `castIfNotSameType`.
3. **`CharVarcharUtils.padCharToTargetLength` — struct padding is
discarded.**
`needPadding = padded.isDefined` overwrote the flag on every field
instead of accumulating
it, so only the *last* field decided whether the struct was rebuilt. A
trailing field that
needs no padding threw away the padding computed for the fields before
it, at any nesting
depth. Changed to `needPadding |= padded.isDefined`.
4. **`CharVarcharUtils.padCharToTargetLength` — struct nullability is lost.**
Rebuilding a struct with `CreateNamedStruct(GetStructField(expr, i),
...)` turns a NULL
struct into a non-NULL struct of NULL fields. Guarded with
`If(IsNull(expr), Literal(null, struct.dataType), struct)`, mirroring
what the scan-side
rewrite in `processStringForCharVarchar` already does a few lines above.
Without this,
fix (3) would have extended the problem to multi-field structs; with it,
the long-standing
single-field case is fixed too.
5. **`CharVarcharUtils.addPaddingInStringComparison` — non-orderable
operands.**
A struct holding a MAP is not comparable, and `CheckAnalysis` rejects it.
Once fix (3)
made such a struct eligible for rebuilding, the error started naming the
rewritten
expression instead of the user's. Comparisons on non-orderable types are
now skipped, so
the message names the original attribute again.
### Why are the changes needed?
All of these produce wrong results or spurious failures, not diagnosable
errors.
**Bugs 1 and 2 together** (`c` is `CHAR(2)` holding `'a'`):
```sql
CREATE TABLE t(c CHAR(2)) USING parquet;
INSERT INTO t VALUES ('a');
SELECT c IN (null, 'a') FROM t;
```
`'a'` matches, and `NULL OR TRUE` is `TRUE` in SQL three-valued logic, so
this must return
`true`. It returns `null`. The analyzed plan before this PR shows the
literal dropped
outright:
```
Filter c#5 IN (null,null) -- 'a' is gone
```
and where a longer literal widens the comparison, the surviving literal is
left unpadded:
```
Filter rpad(c#5, 3, ) IN (rpad(cast(null as string), 3, ), a, null)
```
On a collated column the same bug fails the query instead:
```sql
CREATE TABLE t(c CHAR(2) COLLATE UTF8_LCASE) USING parquet;
SELECT c IN (null, 'A') FROM t;
-- [DATATYPE_MISMATCH.DATA_DIFF_TYPES] ... Input to `in` should all be the
same type,
-- but it's ["STRING COLLATE UTF8_LCASE", "STRING COLLATE UTF8_LCASE",
"STRING"]
```
**Bug 3:**
```sql
CREATE TABLE t(c1 STRUCT<c: CHAR(2), i: INT>, c2 STRUCT<c: CHAR(5), i: INT>)
USING parquet;
INSERT INTO t VALUES (struct('a', 1), struct('a', 1));
SELECT c1 = c2, c1 < c2 FROM t;
```
Both structs hold the same logical value, so this must return `true, false`.
It returns
`false, true`, because `c1.c` is compared as `'a '` against `'a '`.
Moving the `INT` field
to the front of the struct hides the bug, which is why the existing
single-field
`STRUCT<c: CHAR(2)>` coverage never caught it.
**Bug 4** (present since SPARK-33480 for single-field structs):
```sql
CREATE TABLE t(s1 STRUCT<c: CHAR(2)>, s2 STRUCT<c: CHAR(5)>) USING parquet;
INSERT INTO t VALUES (null, null);
SELECT s1 <=> s2 FROM t; -- returns false; NULL <=> NULL must be true
```
Bugs 1, 3 and 4 date back to SPARK-33480 / SPARK-34233 (Spark 3.1) and are
independent of
`spark.sql.charVarchar.standardSemantics.enabled` — they reproduce with the
flag off. Under
that flag the padding rewrite is not the comparison mechanism at all (CHAR
is promoted to
STRING, and `c = 'a'` is already `false` there), so `IN` stays consistent
with `=`.
### Does this PR introduce _any_ user-facing change?
Yes. Queries that returned wrong results, or failed, now behave correctly:
| Query | Before | After |
|---|---|---|
| `c IN (null, 'A')` on `CHAR(2) COLLATE UTF8_LCASE` |
`DATATYPE_MISMATCH.DATA_DIFF_TYPES` | `true` |
| `c IN (null, 'a')` on `CHAR(2)` = `'a'` | `null` | `true` |
| `c IN (null, 'a', 'bcd')` | `null` | `true` |
| `STRUCT<c: CHAR(2), i: INT> = STRUCT<c: CHAR(5), i: INT>`, equal values |
`false` | `true` |
| `s1 <=> s2`, both NULL `STRUCT<c: CHAR(2)>` | `false` | `true` |
| `s1 = s2` where `s1` is NULL | `false` | `null` |
One error message improves: comparing `STRUCT<c: CHAR(2), m:
MAP<STRING,STRING>>` reports
`Cannot resolve "(s1 = s2)"` rather than the rewritten `named_struct(...)`
expression.
Apart from those results and that one message, the only other change is
analyzed-plan text:
fix (2) removes redundant same-type `Cast` nodes from any `In` with
heterogeneous element
types, e.g. `cast(a as int) IN (cast(null as int))` becomes `a IN (cast(null
as int))`. That
is why six golden `analyzer-results` files are regenerated. No golden
*result* file changed
anywhere in the repo.
### How was this patch tested?
Four new tests in `CharVarcharTestSuite`, in the shared trait so they run
under the
file-source, DSV2 and Hive suites:
- `char type IN list with a NULL ahead of the matching literal` — both
spellings of NULL
(`null` and `cast(null as string)`), NULL before / after / interleaved,
matching and
non-matching literals, literals that widen the comparison length,
partitioned and
non-partitioned tables, `spark.sql.readSideCharPadding=false` (the
predicate-padding path),
`NOT IN`, a correlated subquery where the value is an `OuterReference`,
and a collated
CHAR column.
- `char type comparison: multi-field struct` — five shapes: CHAR field first
with a non-CHAR
field last, the reverse order as a control, an all-CHAR struct where only
the first field
needs padding, a multi-field struct nested inside another struct, and one
inside an array.
- `char type comparison: struct nullability is preserved` — NULL structs on
either or both
sides for `=` and `<=>`, single-field and multi-field asserted to agree,
plus NULL arrays
and arrays containing NULL structs.
- `char type comparison: non-orderable struct keeps its original error` —
asserts the
reported `sqlExpr` is the user's expression.
Each was confirmed to fail before the corresponding fix and pass after.
Existing suites, all green:
| Run | Result |
|---|---|
| `sql/testOnly *CharVarchar*` | 719 passed |
| `hive/testOnly *CharVarchar*` | 75 passed |
| `catalyst/testOnly *TypeCoercionSuite *AnsiTypeCoercionSuite
*AnalysisSuite *FilterPushdownSuite *OptimizeInSuite` | 707 passed |
| `sql/testOnly org.apache.spark.sql.SQLQueryTestSuite` | 788 passed |
| `sql/testOnly *PlanStability*` (incl. TPCDS) | 322 passed, no plan churn |
`scalastyle` clean for `catalyst` and `sql` (main and test).
### Was this patch authored or co-authored using generative AI tooling?
No
--
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]
