andygrove opened a new issue, #6628:
URL: https://github.com/apache/datafusion-comet/issues/6628
Part of #6615.
## What / Why
This issue moves array expression coverage to Comet SQL tests under
`sql-tests/expressions/array/`. The sources are `CometArrayExpressionSuite`,
`CometCodegenHOFSuite` (higher-order functions through the codegen dispatcher),
and the array tests in `CometExpressionSuite` (`CreateArray`, `ListExtract`,
`GetArrayStructFields`). Generators (`explode` and friends) and maps have their
own issues. Static analysis on `main` at 3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometArrayExpressionSuite` | 3 | 39 | 23 | 3 | 4 | 72 |
| `CometCodegenHOFSuite` | 0 | 1 | 0 | 4 | 0 | 5 |
| `CometExpressionSuite` | 0 | 3 | 0 | 1 | 0 | 4 |
| **Total** | **3** | **43** | **23** | **8** | **4** | **81** |
Three things recur across this area.
1. **Most array fixtures run one-row batches.**
- They insert 3-6 rows with `INSERT INTO t VALUES`, which under
`local[5]` writes one file per row. So no fixture has a NULL array sharing a
batch with non-NULL arrays.
- The Scala tests read single Parquet files of 100-10000 rows, and
`makeParquetFileAllPrimitiveTypes` makes about half the rows all-NULL.
- Most conversions therefore need `INSERT INTO t SELECT /*+ COALESCE(1)
*/ * FROM VALUES ...` or `... FROM range(0, n, 1, 1)`.
2. **Folded literals are blocked.** Many tests exercise `CometLiteral`'s
handling of folded `ArrayType` / `MapType` / `StructType` literals, which only
exist when ConstantFolding runs. These wait on #6618.
3. **Type breadth.** For the type-breadth tests, put one column per
primitive type in the fixture, each carrying:
- integral types: NULL, 0, Min and Max per width;
- float and double: ±Infinity, NaN, and `float('-0.0')` /
`double('-0.0')`;
- decimals in [0,1);
- dates and timestamps near year 3333;
- strings: numeric-looking, ASCII, a repeated value, multibyte and
supplementary.
With the native scan, the Scala dictionary on/off loops add nothing,
because the native scan decodes dictionary pages before expressions run. On the
convert-from-Parquet path they do matter; see below.
## Work
Line numbers refer to `CometArrayExpressionSuite` unless marked.
**Existing fixtures to extend**
- `array_set_signed_zero_spark_3.sql`,
`array_set_signed_zero_spark_4_0_4_1.sql`, plus a new
`array_set_signed_zero_default.sql` (`MinSparkVersion: 4.2`, no
allowIncompatible) (L65, L83, L113, L135)
- In the files up to 4.1, add `expect_fallback(SPARK-54918)` for nested
element types, negated-NaN distinct/union and nested ±0.0, plus int and string
controls without the opt-in.
- In the 4.2 file, add the same queries as plain `query`, plus literal
0.0/-0.0 distinct/union.
- `array_remove.sql` (L161, L181, L252)
- Rewrite the int shapes as one-file inserts.
- Cover the 13 admitted element types.
- Add a struct element with `expect_fallback(data type not supported)`.
- `array_append.sql` / `array_prepend.sql` (L269 / L302)
- Add boolean, float-widened-to-double, string and timestamp elements.
- Use one-file inserts and a CASE WHEN NULL array.
- `array_insert.sql` (L332): per-row `array(i, NULL, i)` with positions 1,
-1, 8, NULL and -8, plus a NULL item.
- `array_contains.sql` (L380, L439, L466)
- Rewrite as a one-file int table.
- Run the literal-argument queries against a multi-row table, including a
NULL `array<double>`.
- `array_max.sql`, `array_min.sql` (L572, L597): one-file CASE WHEN NULL
arrays, plus literal arguments against the table.
- `arrays_overlap.sql` (L734): a NULL top-level array sharing a batch with
non-NULL arrays, written as one file.
- `array_compact.sql` (L935): rewrite as one file.
- `array_except.sql` (L954, L976): timestamp arrays and the 13 element types.
- `flatten.sql` (L1077): the 13 element types.
- `array_reverse.sql` (L1194, L1209)
- An `array<binary>` table with `expect_dispatch(reverse)`.
- One array column for each of the 14 primitive element types.
- `size.sql` (L1233)
- One-file arrays of varying length, empty and NULL.
- Map CASE shapes; see also the map issue.
- The comment saying "no CreateMap serde" is stale.
- `array_size.sql` (L1297)
- Add the sizeOfNull matrix and a query that includes the NULL row.
- **Un-ignore line 36.** Issue #4560 was fixed by #4578 the day after the
fixture landed.
- `slice.sql` (L1691): `slice` over `split`, `sequence` and `concat`.
- `get_array_item_ansi.sql` (L1704): `split(s, ',')[0]` over `('a,b,c')` and
`(NULL)`.
- `create_array.sql` (`CometExpressionSuite` L3300)
- Integral widening: tinyint, smallint, int and bigint children.
- String elements with a NULL literal, `array(array(s))`, and arrays of
`struct(i)` and `struct(s AS a)`.
- `get_array_item*.sql` / `element_at*.sql` (`CometExpressionSuite` L3322)
- String arrays with NULL elements.
- Non-ANSI `arr[-1]` and `arr[3]`; `element_at` at ±4.
- `element_at(arr, 0)` with `expect_error(INVALID_INDEX_OF_ZERO)`, which
throws even without ANSI.
- `get_array_struct_fields.sql` (`CometExpressionSuite` L3391)
- Nested struct-in-struct field extraction and multiple field types.
- Change the existing query from `spark_answer_only` to `query`, since
`CometGetArrayStructFields` has no restriction.
- `exists.sql` (HOF L99): switch to `expect_dispatch(exists)`. The data and
the ConfigMatrix are already there.
- `transform.sql` (HOF L61, L74, L113) and `array_filter.sql` (HOF L87)
- `expect_dispatch(transform)`, `array_max(transform(...))`,
`array_max(filter(...))` with `expect_dispatch(filter)`.
- Add two collects of a dispatched HOF across concurrent tasks.
- The ScalaUDF wrappers probably don't need to stay. The
codegen-dispatcher analysis found that a built-in parent that always
dispatches, such as `map('k', array_max(transform(...)))`, puts the whole
subtree in one kernel the way the identity UDF did.
- `routing_collection_collation_disabled.sql` (L754): collated
`array_min`/`array_max` fallback with the dispatcher disabled.
**New fixtures**
- `array_contains_types.sql` (L397): allowIncompatible=true, the
type-breadth columns, and complex types as `spark_answer_only`.
- `array_distinct_default.sql`, `array_union_default.sql` (L530, L553)
- The same shapes without allowIncompatible.
- The existing files set the opt-in for the whole file, which hides the
default verdict.
- `arrays_overlap_null_matrix.sql` (L855, L884, L909): the full 16×16, 11×11
and 10×10 literal cross products, plus a NULL column.
- `array_insert_unsupported_args.sql` (L357)
- Dispatcher off, with a position from a dispatch-only child such as a
higher-order function.
- Assert `expect_fallback(unsupported arguments for ArrayInsert)`.
- **Convert-from-Parquet fixtures:** `array_remove_convert_parquet.sql`
(L218), `array_contains_convert_parquet.sql` (L498),
`array_except_convert_parquet.sql` (L1011, with
`expect_dispatch(array_except)`), `flatten_convert_parquet.sql` (L1107),
`array_reverse_convert_parquet.sql` (L1158)
- Use `-- Config: spark.comet.scan.enabled=false` plus
`spark.comet.convert.parquet.enabled=true`.
- Add `-- ConfigMatrix: parquet.enable.dictionary=false,true`; Spark's
reader does hand dictionary vectors to the converter.
- `CometSparkToColumnarExec` converts only `array<string>` among array
types. So in the Scala tests the other element types, struct-of-arrays and
array-of-struct compared Spark with Spark. Test `array<string>` here and drop
the rest.
**Blocked on #6618 (keep ConstantFolding or exclude
`ConvertToLocalRelation`)**
- **Folded nested-array literals into `create_array.sql`:** L1142, L1392,
L1404, L1412, L1423, L1432, L1443, L1563, L1603. The suite comment at
L1386-L1391 says the folded-literal expansion in `CometLiteral` is the code
under test.
- **Folded map and struct literals:** L1452, L1486, L1508, L1577, L1592,
L1615, L1632, L1647, and the folding half of L1464. They live in this suite but
target `map/create_map.sql`, `map/element_at_map.sql`,
`map/folded_map_literal_dedup.sql` and `struct/create_named_struct.sql`.
- **L1535:** a malformed-UTF-8 string literal exists only after folding
`CAST(X'FF' AS STRING)`.
- **The folding halves of the SPLIT tests:** L643 (folded typed-NULL
replacement) and L674.
- **L1668, L1676:** need the non-null-element LocalTableScan source (#4789),
i.e. `ConvertToLocalRelation` excluded.
**Blocked on #6617 (error parity)**
- L1315, L1338 and L1362, the `get_array_item` / `element_at` errors from
#3375.
- `expect_error` already covers the error classes. Meanwhile, add the
message text and query context as patterns.
## Stays in Scala
- **Serde unit internals.** L41 tests the SPARK-54918 version predicate over
patch releases outside the build matrix. L708 inspects the serialized proto.
- **Raw NaN bits.** L775 and L828 check the bits of sign-flipped NaNs;
Parquet canonicalizes NaN, and the SQL comparator canonicalizes it too.
- **The `typedLit` lookup in L1464**, which is DataFrame-only.
- **The ScalaUDF wrappers in the HOF tests,** unless the
always-dispatching-parent approach above replaces them.
## Fixture problems to fix along the way
- **Dispatch claims that aren't pinned.** `transform.sql`, `exists.sql`,
`array_join_eager_eval_dispatch.sql` and `array_join_collation.sql` say they
run through the dispatcher but use plain `query`.
- **No-op configs.** The `allowIncompatible` settings for `ArrayRemove`,
`ArrayInsert` and `ArrayRepeat` do nothing in both fixtures and Scala tests,
because those serdes never return Incompatible.
## Done when
- Every test in the table below is accounted for. Either a fixture covers it
(the same inputs, configs and assertion strength, or stronger), the fixture
that already covers it is cited, or it stays in Scala for the reason given
above.
- Each converted Scala test is deleted in the PR that adds its coverage. A
suite left empty is deleted and removed from the suite lists in
`.github/workflows/pr_build_linux.yml` and `pr_build_macos.yml`.
- The new and changed fixtures pass on every Spark profile. Apply
`run-all-spark-profiles`, because pull requests run only the default profile.
<details><summary>Every Scala test in scope, with its verdict and target
fixture</summary>
| Suite | Line | Test | Verdict | Target |
|---|---|---|---|---|
| `CometArrayExpressionSuite` | 41 | array set signed-zero Spark patch
versions | keep | |
| `CometArrayExpressionSuite` | 65 | array set signed-zero support levels |
convert | array/array_set_signed_zero_spark_3.sql;
array/array_set_signed_zero_spark_4_0_4_1.sql;
array/array_set_signed_zero_default.sql (new) |
| `CometArrayExpressionSuite` | 83 | array set signed-zero fallback and
native opt-in | convert | array/array_set_signed_zero_default.sql (new) |
| `CometArrayExpressionSuite` | 113 | array set noncanonical NaN
normalization | convert | array/array_set_signed_zero_spark_3.sql;
array/array_set_signed_zero_spark_4_0_4_1.sql;
array/array_set_signed_zero_default.sql (new) |
| `CometArrayExpressionSuite` | 135 | array set nested signed-zero
normalization | convert | array/array_set_signed_zero_spark_3.sql;
array/array_set_signed_zero_spark_4_0_4_1.sql;
array/array_set_signed_zero_default.sql (new) |
| `CometArrayExpressionSuite` | 161 | array_remove - integer | convert |
array/array_remove.sql |
| `CometArrayExpressionSuite` | 181 | array_remove - test all types (native
Parquet reader) | convert | array/array_remove.sql |
| `CometArrayExpressionSuite` | 218 | array_remove - test all types (convert
from Parquet) | convert | array/array_remove_convert_parquet.sql (new) |
| `CometArrayExpressionSuite` | 252 | array_remove - fallback for
unsupported type struct | convert | array/array_remove.sql |
| `CometArrayExpressionSuite` | 269 | array_append | convert |
array/array_append.sql |
| `CometArrayExpressionSuite` | 302 | array_prepend | convert |
array/array_prepend.sql |
| `CometArrayExpressionSuite` | 332 | ArrayInsert | convert |
array/array_insert.sql |
| `CometArrayExpressionSuite` | 357 | ArrayInsertUnsupportedArgs | convert |
array/array_insert_unsupported_args.sql (new) |
| `CometArrayExpressionSuite` | 380 | array_contains - int values | convert
| array/array_contains.sql |
| `CometArrayExpressionSuite` | 397 | array_contains - test all types
(native Parquet reader) | convert | array/array_contains_types.sql (new) |
| `CometArrayExpressionSuite` | 439 | array_contains - array literals |
convert | array/array_contains.sql |
| `CometArrayExpressionSuite` | 466 | array_contains - NULL array returns
NULL | convert | array/array_contains.sql |
| `CometArrayExpressionSuite` | 498 | array_contains - test all types
(convert from Parquet) | convert | array/array_contains_convert_parquet.sql
(new) |
| `CometArrayExpressionSuite` | 530 | array_distinct | convert |
array/array_distinct_default.sql (new) |
| `CometArrayExpressionSuite` | 553 | array_union | convert |
array/array_union_default.sql (new) |
| `CometArrayExpressionSuite` | 572 | array_max | convert |
array/array_max.sql |
| `CometArrayExpressionSuite` | 597 | array_min | convert |
array/array_min.sql |
| `CometArrayExpressionSuite` | 622 | array_intersect | covered |
array/array_intersect.sql |
| `CometArrayExpressionSuite` | 643 | array_join | split (rest: #6618) |
array/array_join.sql |
| `CometArrayExpressionSuite` | 674 | array_join support level pins the
native path | split (rest: #6618) | array/array_join_eager_eval_dispatch.sql;
array/array_join.sql |
| `CometArrayExpressionSuite` | 708 | array_join guards only a nullable
replacement | keep | |
| `CometArrayExpressionSuite` | 734 | arrays_overlap | convert |
array/arrays_overlap.sql |
| `CometArrayExpressionSuite` | 754 | array extrema - collations fall back
when the dispatcher is disabled | convert |
array/routing_collection_collation_disabled.sql |
| `CometArrayExpressionSuite` | 775 | array extrema - runtime NaN
representations | keep | |
| `CometArrayExpressionSuite` | 828 | arrays_overlap - runtime NaN
representations | keep | |
| `CometArrayExpressionSuite` | 855 | arrays_overlap - null handling
behavior verification | convert | array/arrays_overlap_null_matrix.sql (new) |
| `CometArrayExpressionSuite` | 884 | arrays_overlap - nested array null
handling behavior verification | convert | array/arrays_overlap_null_matrix.sql
(new) |
| `CometArrayExpressionSuite` | 909 | arrays_overlap - struct element null
handling behavior verification | convert | array/arrays_overlap_null_matrix.sql
(new) |
| `CometArrayExpressionSuite` | 935 | array_compact | convert |
array/array_compact.sql |
| `CometArrayExpressionSuite` | 954 | array_except - basic test (only
integer values) | convert | array/array_except.sql |
| `CometArrayExpressionSuite` | 976 | array_except - test all types (native
Parquet reader) | convert | array/array_except.sql |
| `CometArrayExpressionSuite` | 1011 | array_except - test all types
(convert from Parquet) | convert | array/array_except_convert_parquet.sql (new)
|
| `CometArrayExpressionSuite` | 1047 | array_repeat | covered |
array/array_repeat.sql |
| `CometArrayExpressionSuite` | 1077 | flatten - test all types (native
Parquet reader) | convert | array/flatten.sql |
| `CometArrayExpressionSuite` | 1107 | flatten - test all types (convert
from Parquet) | convert | array/flatten_convert_parquet.sql (new) |
| `CometArrayExpressionSuite` | 1142 | array literals | blocked: #6618 |
array/create_array.sql |
| `CometArrayExpressionSuite` | 1158 | array_reverse | convert |
array/array_reverse_convert_parquet.sql (new) |
| `CometArrayExpressionSuite` | 1194 | array_reverse - binary array |
convert | array/array_reverse.sql |
| `CometArrayExpressionSuite` | 1209 | array_reverse 2 | convert |
array/array_reverse.sql |
| `CometArrayExpressionSuite` | 1233 | size with array input | convert |
array/size.sql |
| `CometArrayExpressionSuite` | 1280 | size - respect to legacySizeOfNull |
covered | array/size.sql |
| `CometArrayExpressionSuite` | 1297 | array_size returns null for null
input | convert | array/array_size.sql |
| `CometArrayExpressionSuite` | 1315 | (ansi) array access out of bounds -
GetArrayItem | blocked: #6617 | array/get_array_item_ansi.sql |
| `CometArrayExpressionSuite` | 1338 | (ansi) array access out of bounds -
element_at with invalid index | blocked: #6617 | array/element_at_ansi.sql |
| `CometArrayExpressionSuite` | 1362 | (ansi) array access with zero index -
element_at | blocked: #6617 | array/element_at_ansi.sql |
| `CometArrayExpressionSuite` | 1392 | array of folded map literals with
array values (multirow) | blocked: #6618 | array/create_array.sql |
| `CometArrayExpressionSuite` | 1404 | folded map with non-null nested array
beside a dynamic map sibling runs natively (multirow) | blocked: #6618 |
array/create_array.sql |
| `CometArrayExpressionSuite` | 1412 | array of folded map literals
(multirow) | blocked: #6618 | array/create_array.sql |
| `CometArrayExpressionSuite` | 1423 | folded map literal keeps non-nullable
values next to a dynamic map sibling (multirow) | blocked: #6618 |
array/create_array.sql |
| `CometArrayExpressionSuite` | 1432 | folded array of maps with a NULL
value (multirow) | blocked: #6618 | array/create_array.sql |
| `CometArrayExpressionSuite` | 1443 | folded array mixing a map literal and
a NULL element (multirow) | blocked: #6618 | array/create_array.sql |
| `CometArrayExpressionSuite` | 1452 | folded map literals with nested map
values (multirow) | blocked: #6618 | map/create_map.sql |
| `CometArrayExpressionSuite` | 1464 | folded empty and NULL complex
literals | split (rest: #6618) | map/create_map.sql |
| `CometArrayExpressionSuite` | 1486 | folded map value with an out-of-range
calendar interval falls back (multirow) | blocked: #6618 | map/create_map.sql |
| `CometArrayExpressionSuite` | 1508 | folded map value with malformed UTF-8
falls back (multirow) | blocked: #6618 | map/create_map.sql |
| `CometArrayExpressionSuite` | 1535 | string literal holding malformed
UTF-8 falls back (multirow) | blocked: #6618 | string/decode_invalid_utf8.sql |
| `CometArrayExpressionSuite` | 1563 | folded array literal whose element
type the list encoder cannot carry falls back | blocked: #6618 |
array/create_array.sql |
| `CometArrayExpressionSuite` | 1577 | folded map value with duplicate
struct field names falls back (multirow) | blocked: #6618 | map/create_map.sql |
| `CometArrayExpressionSuite` | 1592 | folded struct literal in multirow
projection falls back | blocked: #6618 | struct/create_named_struct.sql |
| `CometArrayExpressionSuite` | 1603 | folded array of structs with
divergent field nullability falls back (multirow) | blocked: #6618 |
array/create_array.sql |
| `CometArrayExpressionSuite` | 1615 | folded map literal with duplicate
keys falls back (multirow) | blocked: #6618 | map/folded_map_literal_dedup.sql
(new) |
| `CometArrayExpressionSuite` | 1632 | folded map literal with duplicate
binary keys falls back (multirow) | blocked: #6618 |
map/folded_map_literal_dedup.sql (new) |
| `CometArrayExpressionSuite` | 1647 | folded map literal with distinct
binary keys (multirow) | blocked: #6618 | map/element_at_map.sql |
| `CometArrayExpressionSuite` | 1668 | slice on non-null element array from
local table scan (#4789) | blocked: #6618 | array/slice.sql |
| `CometArrayExpressionSuite` | 1676 | array_insert on non-null element
array from local table scan (#4789) | blocked: #6618 | array/array_insert.sql |
| `CometArrayExpressionSuite` | 1691 | slice over expression-produced
non-null element arrays (#5743) | convert | array/slice.sql |
| `CometArrayExpressionSuite` | 1704 | (ansi) GetArrayItem on null array
from split() | convert | array/get_array_item_ansi.sql |
| `CometCodegenHOFSuite` | 61 | ArrayTransform inside identity ScalaUDF over
Array<Int> | split (rest: other) | array/transform.sql |
| `CometCodegenHOFSuite` | 74 | array_max over ArrayTransform inside
identity ScalaUDF | split (rest: other) | array/transform.sql |
| `CometCodegenHOFSuite` | 87 | array_max over ArrayFilter inside identity
ScalaUDF | split (rest: other) | array/array_filter.sql |
| `CometCodegenHOFSuite` | 99 | ArrayExists respects three-valued logic
through codegen dispatcher | convert | array/exists.sql |
| `CometCodegenHOFSuite` | 113 | HOF query produces correct results across
two collects (per-task isolation regression) | split | array/transform.sql |
| `CometExpressionSuite` | 3272 | read array[int] from parquet | split
(rest: other) | array/element_at.sql |
| `CometExpressionSuite` | 3300 | CreateArray | convert |
array/create_array.sql |
| `CometExpressionSuite` | 3322 | ListExtract | convert |
array/get_array_item.sql + array/element_at.sql + array/get_array_item_ansi.sql
+ array/element_at_ansi.sql |
| `CometExpressionSuite` | 3391 | GetArrayStructFields | convert |
array/get_array_struct_fields.sql |
</details>
--
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]
