srielau commented on PR #58581:
URL: https://github.com/apache/spark/pull/58581#issuecomment-5717198701
## Agent working brief (scope freeze)
This PR has been looping because reviews (public GitHub comments and silent
internal-review commits) keep converting adjacent cases into required work.
**Do not continue that loop.**
CHAR and VARCHAR are SQL types. Support them where Spark already carries
ordinary SQL types. Do not invent CHAR/VARCHAR support for every container,
encoder, or Arrow path that can hold a `DataType`.
### Mission
Ship **ordinary CHAR/VARCHAR support** through Spark SQL type representation
and Spark Connect local-relation transport.
### Success condition
Merge when all of the following are true:
1. A Connect client can `createDataFrame` with CHAR/VARCHAR (uncollated,
explicit `UTF8_BINARY`, and a non-binary collation).
2. Logical schema keeps length and collation.
3. Physical Arrow values are STRING; the client does not apply CHAR padding.
4. The server applies standard, default, and `legacy.charVarcharAsString`
policy once, for empty and populated input, identically.
5. Ordinary nested types work: struct field, array element, map key, map
value.
6. Schema JSON for those ordinary cases is readable by the immediately
preceding JVM reader: uncollated `char(n)`/`varchar(n)` plus ignorable metadata.
7. Scala and Python agree on encode/decode and on rejection of conflicting
encodings.
8. This PR does **not** change ordinary UDT serialization, public
`StringType` constructor identity, or global Arrow/pandas CHAR/VARCHAR
acceptance.
If a later review asks for anything outside this list, reply that it is a
follow-up, do not implement it here, and stop.
### Fence
A change is in this PR only if all three hold:
1. A user can declare or observe CHAR/VARCHAR through SQL, DataFrame schema,
or Connect schema.
2. Classic Spark or established CHAR/VARCHAR semantics are wrong without the
change.
3. The fix does not change the contract of an unrelated type or API.
Test question: **Would this still be required if UDTs, pandas, generic Arrow
UDFs, and explicit ordinary `StringType("UTF8_BINARY")` identity did not
exist?** If no, it is out of scope.
#### In scope
- CHAR/VARCHAR length and optional collation on the type itself.
- DDL, JSON, `simpleString`, `repr`, proto, Scala/Python conversion.
- Ordinary nesting: top-level, struct, array, map.
- Connect: requested **logical** schema stays CHAR/VARCHAR; **physical**
encoding uses STRING; server owns policy.
- Compatibility metadata so old readers parse uncollated CHAR/VARCHAR and
keep unknown keys.
- Collision-safety for that metadata key: do not destroy caller metadata.
- Symmetric Scala/Python validation of that metadata.
- Reverting regressions this branch introduced (UDT unwrap, global Arrow
CHAR/VARCHAR, `StringType` sentinel).
#### Out of scope (separate features / follow-up JIRAs)
- CHAR/VARCHAR inside `UserDefinedType.sqlType`, including JSON path
traversal through UDTs.
- New UDT serialization/restoration design, except **undoing** a break this
PR caused.
- Hidden or public identity of omitted vs explicit `UTF8_BINARY` on ordinary
`StringType`.
- Changing `StringType()` constructor default, `__slots__`, or documented
signature.
- Global pandas / PyArrow / Arrow UDF acceptance of CHAR/VARCHAR.
- Exhaustive cached/chunked/pandas/pyarrow.Table matrices unless this PR
still modifies those paths after the scope reset. If it does, revert the path
change rather than adding tests to keep it.
### Required architecture
Keep one contract. Do not grow extra helpers that then need their own
completeness proofs.
```text
Requested SQL schema (logical CHAR/VARCHAR, collations kept)
|
+-------------------------------+
| |
v v
Physical encoding Protocol logical schema
CHAR/VARCHAR -> STRING CHAR/VARCHAR unchanged
Do NOT unwrap UDTs collations unchanged
Do NOT serialize via sqlType encoder empty and populated share this schema
| |
+------------- server ----------+
|
apply CHAR/VARCHAR policy once
|
result schema
```
Rules:
- Physical lowering is a **Connect transport detail**, not a global
`DataType` rewrite.
- Do not put UDT unwrap inside `DataType.localDataPhysicalType` (or
equivalent) if user objects still need `UDTEncoder`.
- Do not make pandas `types.py` treat CHAR/VARCHAR as Arrow strings globally
in order to fix Connect.
- Prefer one recursive CHAR/VARCHAR collation codec for struct/array/map in
both languages. Do not special-case every call site.
### What to undo on current head
Treat these as scope leaks, not unfinished product work:
1. **`DataType.localDataPhysicalType` UDT unwrap** and JVM `createDataFrame`
using that physical schema with unmodified user objects. Restore UDT
serialization. CHAR/VARCHAR lowering to collation-preserving STRING may remain
**without** unwrapping UDTs.
2. **Global Arrow mapping** in `python/pyspark/sql/pandas/types.py` and
generic conversion that makes `VarcharType` Arrow-legal outside Connect.
Classic `test_err_return_type` must still reject CHAR/VARCHAR if that was prior
behavior.
3. **`StringType` `object()` / `_collation_explicit` sentinel.** Revert
public constructor identity. CHAR/VARCHAR keep `collation=None` vs explicit
name; ordinary STRING stays as before this PR.
4. **Tests whose only job is UDT-in-CHAR JSON, pandas/PyArrow
createDataFrame CHAR, chunked-cache CHAR, or StringType explicitness.** Delete
them with the features.
5. **Scalafmt / line-length / json-module shadowing nits** only as needed
for remaining code. Do not land style-only commits as "review fixes."
Keep:
- Proto optional CHAR/VARCHAR collation fields.
- `__CHAR_VARCHAR_COLLATIONS` (or one dedicated key) for ordinary nested
CHAR/VARCHAR, not `__COLLATIONS` on `char(n)` (old JVM rejects that).
- Logical vs physical Connect split for CHAR/VARCHAR.
- Server policy for empty and populated, including
`legacy.charVarcharAsString`.
- Python CHAR/VARCHAR handled **before** generic `StringType` in JSON (they
are not Python `StringType` subclasses; do not make them subclasses).
### Tests that belong here
Invariant tests, not a growing matrix:
- Direct and nested (struct/array/map) CHAR/VARCHAR JSON: omitted collation,
explicit `UTF8_BINARY`, non-binary collation.
- Preceding-reader shape: uncollated `char`/`varchar` type strings;
collation only in the dedicated metadata key.
- Conflicting inline + metadata encodings rejected in **both** Scala and
Python.
- Connect `createDataFrame` empty and one-row, standard / default /
legacy-as-string.
- Nested struct/array/map in that Connect path.
- Caller metadata that already uses the new key is preserved or the write is
rejected; it must not silently drop or duplicate.
Do not add a test because an agent mentioned a path. Add a test because the
remaining production code has a branch that this PR still owns.
### How to handle reviews
There are two agent loops:
1. Public GitHub reviews posted as `cloud-fan` with `SPARK_DEV_REVIEW_CODE`
markers.
2. Internal review that produces commits and no PR comments.
Both are optional input, not a backlog you must empty.
For each finding, classify:
| Class | Action |
|---|---|
| In-scope correctness bug in remaining CHAR/VARCHAR Connect/JSON code | Fix
it. |
| Regression this branch introduced (UDT encode, Arrow UDF, StringType
signature) | Revert or restore prior behavior. Do not "complete" the new
behavior. |
| Adjacent completeness (UDT.sqlType, pandas, chunked, StringType identity)
| Do not implement. Comment: follow-up, out of fence. |
| Style/comment/repr-coverage nits on out-of-scope APIs | Ignore or
follow-up. |
| "Late catch" that expands the type visitor to a new Spark concept |
Follow-up unless it is ordinary struct/array/map. |
**Do not run an internal-review pass that implements findings without
posting the classification.** If you disagree with a finding, say so on the PR
and stop. Silent fix commits are how this PR ballooned.
Do not force-push unless explicitly asked.
### Stop conditions (mandatory)
Stop and ask instead of coding when:
- A review demands UDT.sqlType collation JSON or ordinary UDT encode/decode
redesign.
- A review demands public `StringType` identity or constructor signature
changes.
- A review demands pandas/PyArrow/Arrow-UDF CHAR/VARCHAR as product support.
- Fixing an in-scope bug seems to require one of the above.
- You are about to add a new helper whose completeness requires recursion
through UDTs or global Arrow.
### Follow-up tickets (mention, do not implement)
1. CHAR/VARCHAR in UDT storage schemas (JSON + encode/decode + Connect).
2. Explicit vs omitted `UTF8_BINARY` identity for ordinary `StringType`.
3. First-class Arrow/pandas CHAR/VARCHAR support.
The next change on this branch should **narrow to this fence**, then wait
for a human before another review-fix cycle.
--
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]
