andygrove opened a new issue, #6629:
URL: https://github.com/apache/datafusion-comet/issues/6629
Part of #6615.
## What / Why
This issue moves map and struct expression coverage to Comet SQL tests under
`sql-tests/expressions/map/` and `sql-tests/expressions/struct/`. The sources
are `CometMapExpressionSuite` and the struct tests in `CometExpressionSuite`
(`named_struct`, `get_struct_field`, `to_json`). Static analysis on `main` at
3bc2faa934 found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometMapExpressionSuite` | 0 | 14 | 15 | 0 | 0 | 29 |
| `CometExpressionSuite` | 1 | 10 | 2 | 2 | 1 | 16 |
| **Total** | **1** | **24** | **17** | **2** | **1** | **45** |
Most of the blocked tests are folded-literal tests. The suite's section
header (L366-L373) says they need ConstantFolding on, so that the map or array
literal reaches `CometLiteral`'s folded-literal expansion and the key-type
guards on the lookup. The SQL suite always excludes ConstantFolding, so these
wait on #6618. As in the array issue, most map fixtures insert 3-6 rows with
`INSERT ... VALUES` (one file per row), so conversions that care about batches
need one-file inserts.
## Work
Line numbers refer to `CometMapExpressionSuite` unless marked.
**Maps: existing fixtures to extend**
- `map_keys.sql`, `map_values.sql` (L36)
- Add `map<bigint,bigint>` with NULL maps and NULL values.
- Change the `spark_answer_only` queries to `query` if the plan is native,
which looks likely.
- `map_from_arrays.sql` (L106): add `array(s)` / `array(i)` columns, and
change the `spark_answer_only` queries to `query`.
- `array/size.sql` (L219, L232): the map shapes.
- `map_from_entries.sql` (L340): an `array<binary>` key/value with
`expect_dispatch(map_from_entries)`.
**Maps: new fixtures**
- `map_struct_keys.sql` (L67): struct-keyed maps with NULLs, including
`map_keys(map1).id2` (#1754).
- `map_from_arrays_ansi.sql` (L130): ANSI on, a codegen matrix, one-file
inserts, and `expect_native(map_from_arrays)`.
- `map_from_arrays_scalar_operands.sql` (L189): a codegen matrix and
`expect_native(map_from_arrays)`.
- `map_from_entries_types.sql` (L264, L299)
- `-- ConfigMatrix: spark.comet.scan.enabled=true,false` plus the convert
config.
- 13 typed columns.
- `map_sort.sql` (L328, L618, `MinSparkVersion: 4.0`): GROUP BY a map, and
GROUP BY a sliced map.
- `map_lookup_sliced.sql` (L593): lookups below `LIMIT 15 OFFSET 5`, with an
ANSI matrix.
- `element_at_map_column_key.sql` (L608): a per-row column key, with an ANSI
matrix.
**Structs** (all lines in `CometExpressionSuite`)
- `struct/get_struct_field.sql`
- Covers L3098, L3115, L3142, and the V1 halves of L3181, L3204 and L3236.
- Use one nested table, `nested1 struct<id, nested2 struct<id>>`, holding
a NULL struct, a NULL inner field and a NULL inner struct.
- Project `nested1.id`, `nested1.nested2`, `nested1.nested2.id`, both
fields together, and `nested1`.
- `struct/get_struct_field_spark_scan.sql` (new, L3069):
`spark.comet.scan.enabled=false` and `spark.comet.convert.parquet.enabled=true`
over the same nested table.
- `struct/create_named_struct.sql`
- L2939: nested `named_struct`, and boolean and tinyint fields.
- L2954: `expect_fallback(with duplicate field names is not supported)`.
- `struct/structs_to_json.sql`
- L2974: non-exact float and double values.
- L2999: CR, LF, form feed, backspace and single-quote characters in
values, and double quotes in field names.
- L3028: supplementary-plane characters, e.g. U+1F911.
**Blocked**
- **#6618, keep ConstantFolding:**
- L383 and L437 into `map_entries.sql`.
- L397, L490, L515, L530 and L545 into `element_at_map.sql`.
- L409, L475 and L501 into `element_at_map_collation.sql`.
- L424 into `create_map.sql`.
- L456 into `map_contains_key.sql`.
- L560 into `conditional/if_nested_nullability.sql`.
- **#6618, exclude `ConvertToLocalRelation`:** L352, the non-null-value
LocalTableScan source from #4789.
- **#6617:** L158, `map_from_arrays` with unequal lengths. It pins native
execution plus error-class, exception-class and SQLSTATE parity.
- **DSv2 halves:** the V2 rows of `CometExpressionSuite` L3069, L3181, L3204
and L3236.
- SQL fixtures over catalog tables always read through the V1 file source.
- The analysis found the V2 halves of L36, L67 and L232 compare Spark with
Spark anyway, because Comet has no native DSv2 Parquet scan and
`convert.parquet` is off.
- The two ignored V2 tests have no issue link. Consider dropping these
halves rather than keeping them.
## Stays in Scala
- **`CometExpressionSuite` L339.** It needs a nullable struct whose inner
field is declared non-nullable, built with `createDataFrame` and an explicit
schema, then pushed through a Comet shuffle. A Parquet table always reads back
nullable fields.
## Fixture problems to fix along the way
- **Lookups that don't use -0.0.** `element_at_map.sql:62-70` and
`get_map_value.sql:48` write `CAST(-0.0 AS DOUBLE)`, which is `+0.0`, so they
never look up a negative zero. They still pass because the fallback is decided
by key type. Use `double('-0.0')`.
- **Duplicates of fixture queries.** L437, L530 and L560 are the same
queries as `map_entries.sql:50-54`, `element_at_map.sql:150-152` and
`if_nested_nullability.sql:77-81`, minus constant folding.
- **Dead branch.** In L219 the `map(_8, _9)` branch never runs, because `_2
= i % 4` is never negative.
## 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 |
|---|---|---|---|---|
| `CometExpressionSuite` | 339 | GetStructField: non-nullable field of a
nullable struct (Delta action-frame shape) | keep | |
| `CometExpressionSuite` | 2939 | named_struct | convert |
struct/create_named_struct.sql |
| `CometExpressionSuite` | 2954 | named_struct with duplicate field names |
convert | struct/create_named_struct.sql |
| `CometExpressionSuite` | 2974 | to_json | convert |
struct/structs_to_json.sql |
| `CometExpressionSuite` | 2999 | to_json escaping of field names and string
values | convert | struct/structs_to_json.sql |
| `CometExpressionSuite` | 3028 | to_json unicode | convert |
struct/structs_to_json.sql |
| `CometExpressionSuite` | 3053 | struct and named_struct with dictionary |
covered | struct/create_named_struct.sql |
| `CometExpressionSuite` | 3069 | get_struct_field | split (rest: other) |
struct/get_struct_field_spark_scan.sql (new) |
| `CometExpressionSuite` | 3098 | get_struct_field - select primitive fields
| convert | struct/get_struct_field.sql |
| `CometExpressionSuite` | 3115 | get_struct_field - select subset of struct
| convert | struct/get_struct_field.sql |
| `CometExpressionSuite` | 3142 | get_struct_field - read entire struct |
convert | struct/get_struct_field.sql |
| `CometExpressionSuite` | 3181 | get_struct_field with DataFusion
ParquetExec - simple case - V1 (testV1AndV2; test( at line 3167) | convert |
struct/get_struct_field.sql |
| `CometExpressionSuite` | 3181 | get_struct_field with DataFusion
ParquetExec - simple case - V2 (ignored; ignore( at line 3176) | blocked: other
| struct/get_struct_field.sql |
| `CometExpressionSuite` | 3204 | get_struct_field with DataFusion
ParquetExec - select subset of struct - V1 (testV1AndV2; test( at line 3167) |
convert | struct/get_struct_field.sql |
| `CometExpressionSuite` | 3204 | get_struct_field with DataFusion
ParquetExec - select subset of struct - V2 (ignored; ignore( at line 3176) |
blocked: other | struct/get_struct_field.sql |
| `CometExpressionSuite` | 3236 | get_struct_field with DataFusion
ParquetExec - read entire struct | split (rest: other) |
struct/get_struct_field.sql |
| `CometMapExpressionSuite` | 36 | read map[int, int] from parquet | convert
| map/map_keys.sql; map/map_values.sql |
| `CometMapExpressionSuite` | 67 | read map[struct, struct] from parquet |
convert | map/map_struct_keys.sql (new) |
| `CometMapExpressionSuite` | 106 | map_from_arrays | convert |
map/map_from_arrays.sql |
| `CometMapExpressionSuite` | 130 | map_from_arrays short-circuits null keys
(codegen=$codegenEnabled) [instances: false, true] | convert |
map/map_from_arrays_ansi.sql (new) |
| `CometMapExpressionSuite` | 158 | map_from_arrays rejects unequal batched
row lengths (codegen=$codegenEnabled) [instances: false, true] | blocked: #6617
| map/map_from_arrays_unequal_lengths.sql (new) |
| `CometMapExpressionSuite` | 189 | map_from_arrays broadcasts scalar
operands (codegen=$codegenEnabled) [instances: false, true] | convert |
map/map_from_arrays_scalar_operands.sql (new) |
| `CometMapExpressionSuite` | 219 | size with map input | convert |
array/size.sql |
| `CometMapExpressionSuite` | 232 | size with map input - v2 reader |
convert | array/size.sql |
| `CometMapExpressionSuite` | 264 | map_from_entries - convert from Parquet
| convert | map/map_from_entries_types.sql (new) |
| `CometMapExpressionSuite` | 299 | map_from_entries - native Parquet reader
| convert | map/map_from_entries_types.sql (new) |
| `CometMapExpressionSuite` | 328 | group by map column with string values |
convert | map/map_sort.sql (new) |
| `CometMapExpressionSuite` | 340 | map_from_entries - binary type routes
through codegen dispatcher | convert | map/map_from_entries.sql |
| `CometMapExpressionSuite` | 352 | map_entries on non-null value map from
local table scan (#4789) | blocked: #6618 | map/map_entries.sql |
| `CometMapExpressionSuite` | 383 | map_entries on a folded
non-nullable-value map (multirow) | blocked: #6618 | map/map_entries.sql |
| `CometMapExpressionSuite` | 397 | nested map lookup with floating-point
keys falls back | blocked: #6618 | map/element_at_map.sql |
| `CometMapExpressionSuite` | 409 | nested map lookup with collated string
keys falls back | blocked: #6618 | map/element_at_map_collation.sql |
| `CometMapExpressionSuite` | 424 | folded map literal with
calendar-interval keys falls back (multirow) | blocked: #6618 |
map/create_map.sql |
| `CometMapExpressionSuite` | 437 | map_entries widening keeps nested map
value nullability (multirow) | blocked: #6618 | map/map_entries.sql |
| `CometMapExpressionSuite` | 456 | map_contains_key over nested
floating-point map keys falls back (multirow) | blocked: #6618 |
map/map_contains_key.sql |
| `CometMapExpressionSuite` | 475 | map_contains_key over nested collated
map keys falls back (multirow) | blocked: #6618 |
map/element_at_map_collation.sql |
| `CometMapExpressionSuite` | 490 | folded map literal with floating-point
keys in element_at falls back (multirow) | blocked: #6618 |
map/element_at_map.sql |
| `CometMapExpressionSuite` | 501 | folded map literal with collated string
keys in element_at falls back (multirow) | blocked: #6618 |
map/element_at_map_collation.sql |
| `CometMapExpressionSuite` | 515 | folded map literal with complex array
key in element_at falls back (multirow) | blocked: #6618 |
map/element_at_map.sql |
| `CometMapExpressionSuite` | 530 | folded nested map lookup with per-row
key evaluation (multirow, non-ansi) | blocked: #6618 | map/element_at_map.sql |
| `CometMapExpressionSuite` | 545 | large folded map literal in element_at
runs natively (multirow) | blocked: #6618 | map/element_at_map.sql |
| `CometMapExpressionSuite` | 560 | IF with a folded map literal branch and
a map column (multirow) | blocked: #6618 |
conditional/if_nested_nullability.sql |
| `CometMapExpressionSuite` | 593 | map lookup on a sliced map reads the
visible entries | convert | map/map_lookup_sliced.sql (new) |
| `CometMapExpressionSuite` | 608 | element_at on a map column with a
per-row lookup key | convert | map/element_at_map_column_key.sql (new) |
| `CometMapExpressionSuite` | 618 | mapsort on a sliced map does not overrun
the sorted entries | convert | map/map_sort.sql (new) |
</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]
