[SPARK-58277][SQL] Stream DataType JSON serialization#57453
[SPARK-58277][SQL] Stream DataType JSON serialization#57453marcuslin123 wants to merge 4 commits into
Conversation
uros-b
left a comment
uros-b
left a comment
There was a problem hiding this comment.
cc @bhollis-dbx regarding #57454 (review).
| assert(DataType.fromDDL("ts timestamp_ltz") == expectedStructType) | ||
| } | ||
|
|
||
| test("SPARK-58277: streaming DataType JSON matches json4s output") { |
There was a problem hiding this comment.
The equivalence test never exercises PythonUserDefinedType.writeJsonTo, whose field layout (type/pyClass/serializedClass/sqlType, omitting the class field the Scala UDT emits) is a genuinely distinct new code path. Only ExampleBaseTypeUDT is covered, and its pyUDT is null (base default), so neither the PythonUDT branch nor any non-null pyClass value is byte-verified against the json4s path. Please add a PythonUDT case (and a non-null-pyClass/null-metadata case) to gate confidence in this new path.
There was a problem hiding this comment.
Added two PythonUserDefinedType cases to the equivalence loop: one with a non-null pyClass and non-null serializedClass, and one with a null serializedClass. Both now byte-verify PythonUserDefinedType.writeJsonTo (the type/pyClass/serializedClass/sqlType layout that omits class) against the json4s output. Confirmed via a mutation check -- corrupting the serializedClass write makes the test fail -- that the new cases actually exercise that path. Pushed in 7be3aa2.
…valence test Add two PythonUserDefinedType cases to the DataTypeSuite streaming-JSON equivalence test so PythonUserDefinedType.writeJsonTo is byte-verified against the json4s path. That path has a distinct field layout from the Scala UDT path (it omits `class` and adds `serializedClass`) and was previously uncovered: the only UDT under test was ExampleBaseTypeUDT, whose pyUDT/serializedPyClass are null. One new case has a non-null pyClass and serializedClass; the other has a null serializedClass to verify null string fields render identically.
…and json4s imports The JsonGenerator import and the org.json4s imports are in the same third-party import group, so scalastyle disallows a blank line between them.
…ons signature Apply scalafmt formatting to StructField.scala: collapse the method signature onto one line and indent the match block, per the project formatter.
HyukjinKwon
left a comment
HyukjinKwon
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 1 nit.
Clean, well-scoped performance refactor. Each streaming writeJsonTo mirrors its jsonValue counterpart, and since both the old and new paths render through the same json4s Jackson backend, byte-for-byte equivalence is grounded structurally, not just by the (strong) regression test.
Nits (1)
- sql/catalyst/src/test/scala/org/apache/spark/sql/types/DataTypeSuite.scala:219 -- the equivalence test's
Doublemetadata values are all non-integral; adding an integral1.0case would cover the canonical number-format spot. -- see inline
Verification
Confirmed the compact-JSON invariant: every writeJsonTo reproduces its jsonValue field names/order/nesting (incl. StructField collation stripping + metadata append, and PythonUserDefinedType's class-omitting layout). prettyJson and error paths still use jsonValue, so nothing is dead. Both the pre-existing path (org.json4s.jackson.JsonMethods) and the new JsonGenerator path share Jackson's number/null/string serialization, so double/null rendering is structurally equivalent. The regression test byte-compares toJson and ObjectMapper.writeValueAsString against the json4s output over atomic + object types (collations, nested metadata, both UDTs, geo, nested struct/array/map).
| .build() | ||
| val metadata = new MetadataBuilder() | ||
| .putString("comment", "field metadata") | ||
| .putDouble("score", 1.5) |
There was a problem hiding this comment.
Minor, non-blocking: the Double values exercised here (score 1.5, weights 2.5/3.5) are all non-integral. An integral-valued double (e.g. .putDouble("whole", 1.0)) is the classic place where JSON number formatting can diverge between renderers (1.0 vs 1). It's structurally safe in this PR since both the old and new paths serialize numbers through the same json4s Jackson backend, but adding a 1.0 case is cheap hardening that would pin that guarantee down in the byte-for-byte comparison.
3 participants
What changes were proposed in this pull request?
This PR changes compact
DataTypeJSON serialization to write directly with a JacksonJsonGeneratorinstead of first building the full json4s AST and then rendering it.It adds streaming writers for primitive data types, arrays, maps, structs, struct fields,
metadata, UDTs, geometry, and geography. The existing
jsonValueandprettyJsonpaths arekept unchanged for compatibility.
Why are the changes needed?
DataType.jsoncan be called on very wide or deeply nested schemas. The previous implementationmaterialized a json4s AST for the whole schema before rendering the final string, which can make
peak driver memory much larger than the final JSON output.
Streaming directly to Jackson keeps serialization proportional to the output being written while
preserving the existing JSON format byte-for-byte.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Added a regression test that compares the streaming output byte-for-byte with the previous json4s
output through both
DataTypeJsonUtils.toJsonand JacksonObjectMapperserialization.Ran:
Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex was used for code assistance.