[SPARK-59278][SQL] Fix CHAR comparison rewrite edge cases

[shivadarshan-devadiga]

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 zipped 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 createStringRPadgives 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'):

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:

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:

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):

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

[shivadarshan-devadiga]

@srielau @cloud-fan Can you please take a look

[cloud-fan]

Review summary

The CHAR comparison fixes are internally consistent and backed by broad regression coverage, including NULL placement and typing, collations, nested and nullable structs and arrays, configuration variants, analyzer plan shape, and the invalid non-orderable path. The remaining P3 follow-up is to correct one test comment so it reflects that castIfNotSameType casts only the untyped NULL rather than every IN operand.

Findings

1 total: 0 P0, 0 P1, 0 P2, 1 P3.

Nit (P3)

• Correct the stale InConversion explanation — sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:514 — see inline.

[cloud-fan]

Nit (P3): castIfNotSameType does not coerce every operand here: c and 'a' are already StringType, so only the untyped NULL gets cast. Please update this explanation to say that the untyped form invokes InConversion, while the pre-typed STRING NULL needs no coercion; the CHAR-backed value stays unchanged in both cases.

[shivadarshan-devadiga]

Thank you for the review @cloud-fan ; I have addressed the nit and made changes accordingly. Can you PTAL

[srielau]

Thanks for picking this up, and sorry for the duplicate.

I independently landed the same JIRA as #58585 after this PR was already open. Your patch covers the same IN-list zip/NULL-order and multi-field struct padding bugs, and it goes further in two places I think are the right design:

• Fixing the redundant same-type Cast in InTypeCoercion so ApplyCharTypePadding still sees the CHAR attribute, rather than peeling analyzer casts later.
• Skipping padding on non-orderable types so CheckAnalysis still names the user's expression.

I will close #58585 as a duplicate in favor of this PR.

One small extra coverage item from #58585, if you want it: with spark.sql.optimizer.inSetConversionThreshold=1, c IN (NULL, 'a', 'b') should still be true and c IN (NULL, 'x', 'y') should still be null. OptimizeIn can freeze a corrupted list into InSet, so that path is worth locking in.

[shivadarshan-devadiga]

Thanks @srielau for closing #58585 in favour of this one and for the review of the design choices.

I've taken your coverage suggestion and added it to the test case:

• With spark.sql.optimizer.inSetConversionThreshold=1, c IN (null, 'a', 'b') is true and c IN (null, 'x', 'y') is null.
• Added a collated variant too, since InSet routes non-UTF8-binary collations through CollationAwareSet, which keys on the collation key rather than binary UTF8String equality.

[dtenedor]

LGTM, merging to master + 4.x

[dtenedor]

Merge Summary:

• merged into master e15ac21
• merged into branch-4.x 51e4436

Posted by merge_spark_pr.py