NestDream commented on PR #59038:
URL: https://github.com/apache/spark/pull/59038#issuecomment-6060026242
Thanks @HyukjinKwon for the review! I've done some research according to the
comments you provided last week.
> The shorter-row branch of the `StructType.toInternal` check has no test
That case went in with a7531fdfa16 (`([(1, d), (2,)], "y long, d date",
"1")` plus the Connect parity case). Moot now: this revision drops the
`toInternal` check, and a shorter row fails in the JVM with
`STRUCT_ARRAY_LENGTH_MISMATCH` as on master, which the new tests assert.
> Keep the "Upgrading from PySpark 4.3 to 4.4" heading; branch-4.x is 4.4.0
and this fix would normally be backported there, so no rename is needed.
The entry is gone in da8f73e0af5 since results don't change anymore; the
`versionchanged` note on `verifySchema` says 4.4.0 for the same reason.
`test_udf_struct_result_length_mismatch` went with the `types.py` guards, that
path is back to master.
Summary of the options, since this went back and forth.
The problem: with an inferred schema, `createDataFrame` pairs values with
fields by position and never looks at `verifySchema`. `[("a", 1), ("b", 2, 3)]`
loses the 3, and `Row(c=3, a="y", b=2)` after `Row(a="x", b=1)` comes back as
`Row(a='3', b=None)`. For string and numeric rows this started in 4.1.0 with
SPARK-52433; 4.0.4 failed in the JVM.
Four variants, each measured on a 938-cell `createDataFrame` matrix plus
eleven small apps that use `createDataFrame` the way people actually do (69
variants, run on master, each candidate, and the 4.0.4 and 4.1.0 wheels):
| option | result changes | who notices | cost |
|---|---|---|---|
| A. Document only | none | nobody, but the data stays wrong and for an
inferred schema there is no option to point at | none |
| B. Warn (pushed, da8f73e0af5) | none, 69 of 69 variants equal master |
nobody, unless the job treats warnings as errors; the data stays wrong, the
user is told once per call (once per partition per job on an RDD, so in the
executor log on a cluster) | one `isinstance` and one `len` per row on the
inferred path |
| C. Raise `FIELD_STRUCT_LENGTH_MISMATCH` for inferred schemas when
`verifySchema` is on | 9 of 69: 4 silently wrong results become errors, 5 JVM
errors become earlier Python errors, the other 60 identical | jobs with ragged
rows under an inferred schema; `verifySchema=False` gets the old result back
per call, `RDD.toDF()` has no such parameter | same, plus a migration note |
| D. C plus `spark.sql.pyspark.legacy.allowRowLengthMismatch.enabled` | as C
with the conf unset; 69 of 69 equal master with it set | as C, but one conf
line restores the old result without touching code, `RDD.toDF()` included | 17
lines in `SQLConf` and a conf to maintain |
None of them fixes a kwargs `Row` with the right number of values in a
different order; that is still paired by position. Fixing it means pairing by
name, which changes `createDataFrame(df.rdd, names)` as a positional rename, so
that should be its own JIRA.
Went with B because it is the only one that cannot fail a job that runs
today. C and D exist as commits if you prefer the error, with or without the
conf; D's default could also be off for inferred schemas, as you suggested in
SPARK-35211.
--
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]
