srielau commented on PR #58549:
URL: https://github.com/apache/spark/pull/58549#issuecomment-5717981303
## Agent working brief (scope freeze)
This PR has the same review loop as #58581: a shared Python/Arrow mapper
admits CHAR/VARCHAR globally, then each review round requires the next consumer
to either enforce assignment semantics or reject the type. **Do not continue
filling that matrix.**
CHAR and VARCHAR are SQL types. At a Python/Arrow boundary, "support" means
values that re-enter Catalyst obey CHAR/VARCHAR assignment under the policy
captured at analysis, not ambient conf at execute.
This is SPARK-59275 / PR 58549. It is **not** Connect schema transport (that
is SPARK-59276 / #58581). Do not change Connect logical/physical schema
handling here except where this PR already touches a Python UDF/UDTF client
check that this fence still owns.
### Success condition
Merge when all of the following are true:
1. With `spark.sql.charVarchar.standardSemantics.enabled=true`, scalar
Python UDFs (row, Arrow-optimized, pandas scalar) accept CHAR/VARCHAR return
types and apply padding / VARCHAR length checks before values re-enter Catalyst.
2. Higher-order-function UDFs that reuse that scalar path (`transform`,
etc.) keep the captured policy through fusion and lambda lift.
3. Explicit-schema local, pandas, and PyArrow DataFrame creation apply the
same recursive assignment checks.
4. `DataFrame.toArrow()` exports CHAR/VARCHAR values as Arrow strings.
5. Nested struct / array / map work on those supported paths.
6. Default and `legacy.charVarcharAsString` behavior does not newly expose
first-class CHAR/VARCHAR where the pre-PR surface was STRING or an error.
7. Every other public eval type this mapper now reaches **recursively
rejects** CHAR/VARCHAR with a deterministic validation error (not unchecked
values).
8. No new configuration is added.
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 surface is in this PR only if all four hold:
1. Users routinely declare CHAR/VARCHAR as a UDF, createDataFrame, or
toArrow type.
2. Without the change, standard-semantics CHAR/VARCHAR is either rejected or
silently wrong.
3. This PR can both **admit** the type and **enforce** assignment (or
toArrow export) on that path.
4. The path is not an extension protocol (UDT, DataSource,
TransformWithState) unless this PR is only **rejecting** it.
Test question: **Would this still be required if Arrow UDTFs, row UDTFs,
Python DataSource, TransformWithState, and CHAR/VARCHAR inside UDT.sqlType did
not exist?** If no, it is out of scope.
#### In scope (admit + enforce)
- Scalar Python UDFs: row, Arrow-optimized, pandas scalar.
- Higher-order function UDFs that reuse that scalar path, including copying
captured policy through fusion and lambda lift.
- Explicit-schema local / pandas / PyArrow DataFrame creation.
- `DataFrame.toArrow()` exporting CHAR/VARCHAR as Arrow strings.
- Nested struct / array / map on those paths.
- Policy: standard semantics on; default/legacy must still look like STRING
(or the pre-PR error).
- Exhaustive **rejection** of CHAR/VARCHAR (recursive) on every other public
eval type the shared mapper now reaches.
#### Out of scope (reject now, follow-up JIRA later)
- Arrow UDTFs and row UDTFs (fixed and analyze-derived).
- Python DataSource return schemas.
- TransformWithState / state-server grouping and state schemas.
- CHAR/VARCHAR inside `UserDefinedType.sqlType`.
- Full assignment enforcement for grouped-map, cogroup, aggregate, and
iterator-agg eval types (unless JVM `stringLengthCheck` already exists on that
exec; if not, reject).
- Connect logical/physical CHAR/VARCHAR transport (PR 58581).
- New configs. Existing CHAR/VARCHAR flags are enough.
Unsupported must mean a **deterministic validation error**, not "the mapper
allows it and the JVM happens to work."
### Required architecture
```text
Python/Arrow boundary
|
+-- supported surface? -- yes --> physical STRING in Arrow
| logical CHAR/VARCHAR in Spark
schema
| assignment check with CAPTURED
policy
|
+-- no --> recursive reject before exec
```
Rules:
- `pandas/types.py` (and similar converters) are **physical encoding**, not
the capability API.
- Capability is an explicit allow-list in `udf.py` / creation / `toArrow`.
Everything else rejects recursively.
- Do not use ambient `SQLConf` at execute for a UDF whose policy was
captured at analysis.
- Do not put UDT unwrap / UDT-storage CHAR semantics into this PR except as
part of rejection, or to undo a break this branch caused.
### What to undo or freeze on current head
Keep scalar UDF + createDataFrame + `toArrow` enforcement and the
captured-policy plumbing those paths need.
Stop adding, and revert if they are only there as product support:
- TransformWithState state-server preflight as CHAR/VARCHAR support (reject
grouping/state CHAR/VARCHAR instead).
- UDT-storage recursion as support (do not invent UDT CHAR assignment here).
- Connect UDTF "complete validation" beyond recursive reject.
- Extra policy flags whose only clients are out-of-scope surfaces.
Tests that belong here:
- One case per **supported** surface x standard / default / legacy.
- Nested struct/array/map on the supported UDF and createDataFrame paths.
- One rejection case per **unsupported** public eval type the mapper can
reach.
Do not add a test because an agent mentioned TWS, analyze-UDTF, UDT.sqlType,
mixed-batch, or RDD-only branches unless that production code remains after the
freeze. If the code is out of scope, delete the path change rather than
covering it.
### How to handle reviews
Public GitHub reviews posted as `cloud-fan` with `SPARK_DEV_REVIEW_CODE`,
and silent internal-review commits, are optional input. They are not a backlog
you must empty.
For each finding, classify:
| Class | Action |
|---|---|
| In-scope bug on scalar UDF, HOF UDF, createDataFrame, or toArrow | Fix it.
|
| Mapper now admits an unsupported eval type | Reject recursively; do not
implement assignment. |
| Regression this branch introduced | Restore prior behavior. Do not
"complete" the new behavior. |
| UDT.sqlType, DataSource, TWS, UDTF assignment, grouped-map completeness |
Follow-up. Comment and stop. |
| Captured vs ambient policy on a **supported** path (fusion, lambda lift) |
Fix it. |
| Captured vs ambient policy only needed for an out-of-scope path | Remove
that path from support. |
**Do not run an internal-review pass that implements findings without
posting the classification.** Silent fix commits are how this PR ballooned. Do
not use `--auto` on this self-PR.
The Sep 8 review already recommended this narrower boundary. Freeze there.
### Stop conditions (mandatory)
Stop and ask instead of coding when:
- A review demands CHAR/VARCHAR assignment for UDTFs, DataSource, or
TransformWithState.
- A review demands UDT.sqlType recursion as support.
- A review demands grouped/cogroup/aggregate CHAR assignment rather than
rejection.
- Fixing an in-scope bug seems to require one of the above.
- You are about to treat `to_arrow_type` as a global capability flag again.
### Follow-up tickets (mention, do not implement)
1. CHAR/VARCHAR assignment for Arrow and row Python UDTFs.
2. Python DataSource CHAR/VARCHAR return schemas.
3. TransformWithState grouping/state CHAR/VARCHAR.
4. CHAR/VARCHAR inside UDT storage types.
5. Grouped-map / cogroup / aggregate CHAR/VARCHAR assignment.
The next change on this branch should **narrow to this fence**, then wait
for a human before another review-fix cycle.
Related: #58581 owns Connect schema transport and must not keep a
conflicting global Arrow CHAR/VARCHAR mapper. This PR owns the mapper, behind
the allow-list above.
--
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]
