Conversation
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed e2e891d5962be4ce0c1647407804d40972fb5e28 against fad6230948032012d676bed23156b7c93dd2fa2e. I found no P1/P2 issue.
Previously, any array field prevented an otherwise eligible Spark leaf input from crossing the Spark-to-Comet boundary. The production change adds one explicit ArrayType(StringType, _) case. It reuses the existing readers and writers for top-level string arrays and arrays inside supported structs. Both array-field and element nullability remain independent. Other arrays, maps, and non-default string collations still fail admission, and the existing empty-struct and duplicate-field-name restrictions remain in place.
The exact StringType match matters on Spark 4.0: equality includes the collation ID and string constraint, so this does not admit arbitrary collated strings. I compared the existing list/string writers with the maintained Spark 3.5 and 4.0 sources. Their class bodies match after excluding formatting and line comments. The string writer uses the UTF-8 buffer's position and byte length, preserving slices and embedded zero bytes. The list writer tracks child elements separately from parent rows and finishes the child vector. The ordinary struct handling preserves null parents without adding array elements.
The reader lifecycle supports this admission. Each output batch receives fresh Arrow buffers while previously exported buffers retain their references. Spark columnar inputs are borrowed, and conversion copies their values into Arrow storage. The new eight-case matrix checks both readers across both nullability flags, nonzero source offsets, empty input batches, all-null output batches, long UTF-8 strings, and 4,097-element arrays. It retains actual Arrow buffers across reader advancement and closure, then checks offsets, validity, child counts, schemas, and values after mutating source storage.
The integration test retains the array payload and nullable parent structs through JSON and Parquet, V1 and V2, and row and vectorized Parquet inputs. It compares answers with Spark, checks the conversion node and native filter/projection, verifies the selected reader path, and carries the arrays through native shuffle. Disabled-conversion controls preserve fallback. The encoding-failure test checks both conversion helpers with a throwing vector and verifies zero outstanding allocation. The limit(1) query exercises early task completion. These cover different failure/lifecycle boundaries and should not be read as exhaustive JNI fault injection.
Validation
Current-head CI passed all 11 new tests on Spark 3.4–4.2. The native build and all five execution-test jobs checked out 398b0d6dec53df7ea511040897b44feb9c430bb6. Its parents are the reviewed base and head, and its entire tree equals the head. The native artifact's upload and five download receipts report the same digest.
The latest snapshot contains 54 successful and 10 skipped checks, with none pending or failed. I verified source equivalence locally and inspected the CI logs. I did not run a separate local product build or independently hash the extracted shared library. Maintained-source inspection covered Spark 3.5 and 4.0. Other-version support here is evidenced by CI execution.
Performance
The new admission check has negligible planning cost. Once conversion is selected, the existing path visits array elements and copies string bytes into independently owned Arrow buffers. Work therefore scales with element count and payload size. Off-heap UTF-8 input can also require the existing intermediate byte-array copy. The change introduces no additional writer layer, repeated conversion, or native algorithm.
This enables a previously rejected boundary and makes no measured speedup claim. The JSON, Parquet, and generic Spark-to-Arrow conversion switches remain default-off. The tests establish compatibility, not a throughput or peak-memory improvement. I do not see a new hot-path implementation change requiring a microbenchmark before this bounded support addition. A workload's net benefit still depends on conversion cost versus downstream native work.
Design
The explicit type case is the simplest change consistent with the requested scope. Keeping source eligibility and configuration checks intact avoids enabling arbitrary intermediate operators. Reusing the common reader/writer path is justified by coverage of both inputs, including native round trips and supported struct placement. The documentation describes the new boundary support and retained restrictions without promising an entirely native query.
Abstraction & complexity
No production abstraction is added. The tests use the existing schema checks, answer/operator assertions, Arrow readers, and buffer-retention APIs. Their additional setup directly exercises ownership and nested validity that ordinary answer comparisons could miss. The small nullability/source matrices keep those dimensions visible, and I found no unnecessary indirection or actionable simplification.
Which issue does this PR close?
Closes #5911.
Rationale for this change
An
ARRAY<STRING>field currently prevents Spark-to-Comet conversion even though the Arrow list and string writers already support it. This blocks otherwise eligible native operators above configured Spark sources.What changes are included in this PR?
Admit binary string arrays through the existing type check, including nullable elements and placement inside supported structs. Reuse the existing readers and writers, and document the supported conversion types.
Add coverage for schema restrictions, JSON/Parquet conversion and native shuffle, plus row and columnar reader tests for list offsets, UTF-8 values, batching, buffer ownership and cleanup.
How are these changes tested?
The two new
CometExecSuitetests and nine string-array cases inCometArrowStreamSuitepass in the local verification run on Spark 4.1.3 / JDK 21 after rebasing onto main, using a native library rebuilt with Rust 1.98.1. Formatting and Scalastyle passed. Cross-version fork CI passed before rebasing.