[SPARK-59275][PYTHON] Complete CHAR/VARCHAR support for Python UDFs and Arrow - #58549
[SPARK-59275][PYTHON] Complete CHAR/VARCHAR support for Python UDFs and Arrow#58549srielau wants to merge 1 commit into
Conversation
cloud-fan
left a comment
There was a problem hiding this comment.
Review summary
The current implementation is not ready because it widens non-scalar pandas/Arrow UDF APIs without adding their required JVM assignment checks, and it changes the documented legacy CHAR/VARCHAR-as-STRING behavior. These can expose invalid logical values or introduce new runtime failures in supported configurations. The large-input Arrow RDD path also gains an unconditional per-row projection/copy, and the added Parquet test does not exercise the new Arrow-backed columnar CHAR/VARCHAR branch.
Findings
4 total: 0 P0, 2 P1, 2 P2, 0 P3.
Blocking (P1)
- Honor legacy CHAR/VARCHAR-as-STRING mode —
sql/core/src/main/scala/org/apache/spark/sql/execution/python/EvaluatePython.scala:212— see inline. - Do not enable unchecked non-scalar Arrow outputs —
python/pyspark/sql/pandas/types.py:138— see inline.
Non-blocking (P2)
- Exercise the Arrow-backed CHAR/VARCHAR branch —
python/pyspark/sql/tests/arrow/test_arrow_python_udf.py:316— see inline. - Keep the direct Arrow RDD path for unconstrained schemas —
sql/core/src/main/scala/org/apache/spark/sql/execution/arrow/ArrowConverters.scala:570— see inline.
Verification
- The changed shared Arrow mapper is called by public non-scalar UDF return-type validation, while their unchanged JVM consumers do not apply stringLengthCheck.
- The added Parquet case and the changed ArrowColumnVector guard select different evaluator paths.
PR metadata suggestions
- Narrow the blanket pandas UDF support claim to scalar UDFs unless assignment checks and tests are added for map, grouped, cogrouped, and aggregate eval types.
- Do not claim Arrow columnar-input coverage until a test uses ArrowBackedDataSourceV2 and exercises the ArrowColumnVector CHAR/VARCHAR output branch.
|
|
||
| case c: CharType => (obj: Any) => nullSafeConvert(obj) { | ||
| case _ => | ||
| CharVarcharCodegenUtils.charTypeWriteSideCheck( |
There was a problem hiding this comment.
Blocking (P1): These new CHAR/VARCHAR branches bypass CharVarcharUtils.shouldApplyWriteSideLengthCheck. With spark.sql.legacy.charVarcharAsString=true and both first-class modes off, a CharType(3) UDF returning "a" is now padded and an over-length VarcharType result now fails, although that mode explicitly promises no padding or length check. Please gate this conversion and the new scalar output projection with the existing helper so the legacy path remains unconstrained STRING while first-class modes retain the checks.
Recommended change: Use the existing write-side-check policy for every new Python scalar conversion and output projection.
Why this works: Evaluate shouldApplyWriteSideLengthCheck once for the active SQLConf and select the unconstrained STRING conversion/projection only when legacy-as-string is active without either first-class mode.
Scope: EvaluatePython.makeFromJava, EvalPythonEvaluatorFactory, and focused legacy-mode regression tests for explicit-schema creation and scalar UDF output.
Compatibility: Restores the documented legacy behavior without changing standard-semantics or preserve-type-info behavior.
Risks: The bypass must remain limited to the exact policy helper result so neither first-class mode loses assignment checks.
Constraints: Reuse CharVarcharUtils.shouldApplyWriteSideLengthCheck rather than duplicating configuration precedence.
Success: Legacy mode preserves unpadded and over-length STRING values, while both first-class modes still pad CHAR and reject over-length CHAR/VARCHAR values.
| elif isinstance(dt, DecimalType): | ||
| arrow_type = pa.decimal128(dt.precision, dt.scale) | ||
| elif isinstance(dt, StringType): | ||
| elif isinstance(dt, (StringType, CharType, VarcharType)): |
There was a problem hiding this comment.
Blocking (P1): This shared mapping is also the capability check for mapInPandas, mapInArrow, grouped/cogrouped map, and aggregate UDFs. Those JVM output consumers still use identity projections and never call stringLengthCheck, so a declared VarcharType(3) can return "abcd" unchanged and an under-length CHAR remains unpadded. Please keep CHAR/VARCHAR rejected for non-scalar eval types until each corresponding output consumer converts from physical STRING and applies the recursive assignment checks.
Recommended change: Limit the new CHAR/VARCHAR acceptance to scalar eval types and other boundaries whose consumers already enforce assignment semantics.
Why this works: Separate Arrow transport mapping from eval-type capability validation and recursively reject CHAR/VARCHAR in non-scalar return schemas before calling the shared mapper.
Scope: PySpark UDF return-type validation and focused negative tests for map, grouped, cogrouped, and aggregate pandas/Arrow eval types.
Compatibility: Preserves newly implemented scalar and DataFrame/Arrow support while restoring the prior unsupported-type failure for non-scalar APIs that cannot yet enforce the contract.
Risks: Capability checks can drift from JVM support if eval-type ownership is not kept explicit.
Constraints: Preserve recursive CHAR/VARCHAR detection inside structs, arrays, and maps. Do not disable the shared mapping used by DataFrame creation, toArrow, or supported scalar UDF paths.
Success: Every accepted CHAR/VARCHAR producer applies recursive padding and overflow checks; non-scalar eval types remain rejected until their JVM output paths provide those semantics.
|
|
||
| with tempfile.TemporaryDirectory() as path: | ||
| self.spark.range(1).write.parquet(path) | ||
| columnar_input = self.spark.read.parquet(path) |
There was a problem hiding this comment.
Non-blocking (P2): A vectorized Parquet scan reaches the evaluator's documented non-Arrow columnar path, so this case never exercises the new isArrow && !hasCharVarcharOutput guard for ArrowColumnVector input. Please add the CHAR/VARCHAR case to ArrowColumnarPythonUDFSuite using its readArrowSource fixture, assert that ArrowEvalPythonExec still has an Arrow-backed columnar child, and cover both padding and over-length rejection.
| largeVarTypes, | ||
| TaskContext.get()) | ||
| val projection = UnsafeProjection.create(checkedAttrs, attrs) | ||
| rows.map(row => projection(row).copy(): InternalRow) |
There was a problem hiding this comment.
Non-blocking (P2): For schemas without CHAR/VARCHAR, checkedAttrs is identical to attrs, but this large-input RDD branch still runs every row through an UnsafeProjection and then deep-copies it. That adds CPU and allocations to all Arrow-backed DataFrame creation above arrowLocalRelationThreshold, even when the feature is unused. Please retain the original direct fromBatchIterator path unless hasCharVarchar(schema) requires the checked projection.
What changes were proposed in this pull request?
Complete first-class CHAR/VARCHAR support at PySpark and Arrow boundaries when
spark.sql.charVarchar.standardSemantics.enabledis true:CharTypeandVarcharTypeto Arrow UTF8, recursively through complex types.results before they re-enter Catalyst.
CHAR/VARCHAR in the Spark schema.
DataFrame creation.
DataFrame.toArrow()to export CHAR/VARCHAR values as Arrow strings.JIRA: https://issues.apache.org/jira/browse/SPARK-59275
Why are the changes needed?
CHAR/VARCHAR are first-class types under standard semantics, but Python and Arrow boundaries
still treated them inconsistently. Arrow UDFs rejected them as unsupported, pickled UDF and
explicit-schema creation paths did not enforce their length rules, and Arrow output validation
compared logical CHAR/VARCHAR against physical STRING.
These gaps allowed unpadded CHAR and over-length VARCHAR values or caused supported queries to
fail. The checks must also recurse through structs, arrays, and maps.
Does this PR introduce any user-facing change?
Yes. With
spark.sql.charVarchar.standardSemantics.enabled=true, Python UDFs, Arrow-optimizedUDFs, pandas UDFs, and explicit-schema DataFrame creation now accept CHAR/VARCHAR and enforce
their assignment semantics.
toArrow()exports these values as Arrow strings. The defaultflag-off behavior is unchanged.
How was this patch tested?
Added PySpark coverage for:
DataFrame.toArrow().Ran:
The PySpark integration tests were not run locally because PyArrow is not installed in this
environment.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Cursor Auto (GPT-5.6)