sunchao opened a new issue, #5911: URL: https://github.com/apache/datafusion-comet/issues/5911
### What is the problem the feature request solves? `CometSparkToColumnarExec` currently rejects every `ArrayType` and `MapType`. Consequently, an otherwise eligible Spark input with an `ARRAY<STRING>` field cannot enter Comet through this conversion boundary, even when the corresponding Spark-to-Comet conversion option is enabled. This is a boundary-specific limitation. Comet already has array representations and native array operations; this issue does not propose adding array support to the engine from scratch. It also does not mean that every query containing a string array runs entirely outside Comet. The effect depends on the physical plan, which columns survive pruning, and whether another eligible native boundary exists. #### Current behavior At public main [`451c99963206fa6bf0387239aa12887a16255516`](https://github.com/apache/datafusion-comet/tree/451c99963206fa6bf0387239aa12887a16255516): - [The conversion-specific type check](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/scala/org/apache/spark/sql/comet/CometSparkToColumnarExec.scala#L140) returns `false` for all arrays and maps. - [The execution rule checks that schema before considering the configured source](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/scala/org/apache/comet/rules/CometExecRule.scala#L1022). The current mechanism applies to eligible leaf inputs, including configured Spark JSON/Parquet scans and other explicitly enabled leaves; it does not convert arbitrary intermediate Spark operators. - The same execution node [selects either a row reader or a Spark columnar-batch reader](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/scala/org/apache/spark/sql/comet/CometSparkToColumnarExec.scala#L96), depending on the child. Relaxing its shared type check affects both paths. For example, consider a Spark JSON scan with schema `id INT, tags ARRAY<STRING>` and these synthetic JSON lines: ```json {"id":1,"tags":null} {"id":2,"tags":[]} {"id":3,"tags":[null]} {"id":4,"tags":["error","","é","東京"]} ``` With Comet otherwise enabled, the intended shape is: ```sql SET spark.comet.convert.json.enabled=true; -- json_tags is a Spark JSON-backed view with the schema above. SELECT id, tags, size(tags) AS tag_count FROM json_tags WHERE id > 0; ``` Today, the string-array field prevents the Spark-to-Comet conversion at this input. The desired result is that the source can cross the conversion boundary and otherwise supported downstream operators can be considered for native execution. This example deliberately retains `tags`: a query that prunes the array out before conversion does not demonstrate this limitation. String arrays are a common representation for tags, labels, and categories. Supporting this one type would remove a specific obstacle for those schemas. A performance gain remains workload-dependent; this request makes no measured speedup claim. ### Describe the potential solution #### Bounded first scope Admit `ARRAY<STRING>` with ordinary binary string semantics, covering both `containsNull=true` and `containsNull=false`, while preserving the independent nullability of the array-valued field. Keep existing source configuration and eligibility rules. In particular, this should not enable JSON conversion by default, add new input operators, or change unrelated array/map support. #### Reuse the existing Arrow representation Several necessary pieces already exist: - [`Utils.toArrowField`](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/scala/org/apache/spark/sql/comet/util/Utils.scala#L185) represents Spark arrays as Arrow lists and carries element nullability into the child field. - [`ArrowWriter.createFieldWriter`](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowWriters.scala#L78) constructs an `ArrayWriter` with a recursively constructed element writer. An array of strings therefore uses a list vector whose child is written by `StringWriter`. - [`ArrayWriter`](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowWriters.scala#L558) tracks each list's offsets and delegates element values/nulls to that child writer. Its `finish` and `reset` operations also visit the child. - [`CometListVector`](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/java/org/apache/comet/vector/CometListVector.java#L33) already exposes an Arrow list through Spark's array interface. These are useful implementation foundations, but they do not establish that changing the type check alone is sufficient. Planner admission, schema agreement, the two reader paths, and buffer ownership need end-to-end validation together. #### Potential implementation approaches **Preferred:** add an explicit string-array case to the conversion type check, reuse the existing list/string writers, and repair only any narrowly demonstrated conversion defects. Test both row and Spark-columnar inputs before admitting the type through the shared gate. **If one reader path requires additional work:** make admission depend on the validated source path, leaving the other path on its existing fallback until covered. A row-only test must not silently enable columnar conversion through the common type check. There is also a recursive-admission boundary to consider. [The shared `DataTypeSupport` implementation recursively checks struct fields](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/scala/org/apache/comet/DataTypeSupport.scala#L58). A new accepted array case can therefore also admit `STRUCT<tags:ARRAY<STRING>>` inside an otherwise supported struct. Cover that placement, including null parent structs, or explicitly preserve fallback for it in the initial implementation. Retain existing duplicate-field-name and unsupported-type checks. #### Acceptance criteria 1. **Prove the new plan path.** Add a Spark JSON input test with conversion explicitly enabled and the array retained in the projected schema. Compare results with Spark, assert that the conversion node is present, and assert that at least one eligible downstream filter/projection executes in Comet. A result-only test could pass entirely through fallback. Keep a disabled-conversion control and exercise supported Spark-version/source variants where their planning paths differ. 2. **Distinguish the null cases.** Test a null array, an empty array, `[null]`, an array containing both null and non-null strings, and all-null input. Include consecutive null/empty rows and nulls at batch boundaries. Test both array-field nullability and element `containsNull`, rather than treating them as one flag. Check list offsets, child value counts, values, and schema agreement with Spark. 3. **Exercise string storage.** Include empty strings, multibyte UTF-8, embedded zero bytes in string values where representable by the fixture, and long strings that force the child vector to grow. Mix different array lengths so element counts differ substantially from row counts. Preserve element order and duplicates. 4. **Cover both input readers and slicing.** Test row-backed inputs, including reusable Spark row/string storage, and supported Spark columnar inputs with nested child vectors. Split one input batch into multiple output batches with nonzero source offsets; include empty input batches. Use a small row batch size so the same fixture crosses several boundaries. Do not split a single logical array across rows or lose its elements at a batch boundary. 5. **Preserve ownership across batches.** Retain an exported output batch while advancing the reader, then verify that its list offsets, validity, and string bytes are unchanged. Output must remain valid under the established Arrow ownership contract when the Spark producer reuses its storage. Closing converted output must not close borrowed Spark input. The [row reader's existing buffer-allocation contract](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/RowArrowReader.scala#L28) explains why in-place reuse of previously exported buffers is unsafe. Cover cleanup on encoding failure and early close/task completion. 6. **Verify the native boundary, not only the Java writer.** Exercise conversion through the normal native input path and back to Spark results. Include the struct placement described above if it is admitted. Add a focused round trip through an already supported native exchange with the string array carried as payload if the new plan exposes that path; reuse existing shuffle/IPC behavior. New shuffle-key support or a new spill implementation is not part of this request. 7. **Keep the change narrow.** Verify that arrays of other element types, arrays of arrays, arrays of structs, maps, and unsupported string collations do not become newly admitted through this conversion gate. Existing behavior outside the gate should remain governed by its existing support checks. Document the newly supported type and any retained source-path restrictions. #### Non-goals - General array/map conversion, arbitrary nesting, or new native array functions. - Converting arbitrary intermediate Spark operators or enabling additional source types. - Changing conversion defaults or bypassing existing fallback/configuration checks. - Zero-copy borrowing of Spark strings, a new string representation, or a byte-target batching redesign. - New native shuffle/spill algorithms, array-valued shuffle keys, or guarantees of a whole-query native plan. ### Additional context Public [PR #5713](https://github.com/apache/datafusion-comet/pull/5713) already added low-level Spark-to-Arrow coverage for nullable array elements, nullable strings, slice offsets, independently owned output, and failure cleanup. Its [nested conversion test](https://github.com/apache/datafusion-comet/blob/451c99963206fa6bf0387239aa12887a16255516/spark/src/test/scala/org/apache/spark/sql/comet/execution/arrow/CometArrowStreamSuite.scala#L346) uses an `ARRAY<INT>` column and a separate string column. This issue builds on that coverage to validate strings as the list's elements and to enable the currently rejected planner path. This is distinct from [#5582](https://github.com/apache/datafusion-comet/issues/5582), which concerns array-expression element-type restrictions, and [#4228](https://github.com/apache/datafusion-comet/issues/4228), which concerns preserving dictionary encoding through native expressions. The source links above pin the observed limitation. The synthetic example describes the intended regression test; it is not a report of a new benchmark run. The expected implementation is one focused conversion-support change with its compatibility and ownership tests, or a source-restricted first change if the two reader paths require separate fixes. -- 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]
