andygrove opened a new issue, #6630:
URL: https://github.com/apache/datafusion-comet/issues/6630
Part of #6615.
## What / Why
This issue moves the generator coverage in `exec/CometGenerateExecSuite` to
Comet SQL tests: `explode`, `explode_outer`, `posexplode`, `posexplode_outer`,
over arrays and struct arrays, across batch boundaries, and under `LIMIT ...
OFFSET`. Fixtures go under `sql-tests/expressions/array/`, next to
`explode.sql` and `posexplode.sql`. Static analysis on `main` at 3bc2faa934
found the following:
| Suite | Covered | Convert | Blocked | Split | Keep | Total |
|---|---|---|---|---|---|---|
| `CometGenerateExecSuite` | 1 | 40 | 0 | 0 | 2 | 43 |
Nearly everything converts. The plan-shape test and the ScalaUDF test stay.
## Work
Line numbers refer to `CometGenerateExecSuite`.
- **`explode_local_table_scan.sql` (new)**, covering the 24 `Seq.toDF` tests
from L114 to L436 except L214, L227 and L393
- Use `-- Config: spark.comet.exec.localTableScan.enabled=true` with
inline `VALUES`.
- The behavior is already in `explode.sql` / `posexplode.sql` over
Parquet. What this file adds is the source: Parquet always reads arrays back
with `containsNull=true`, while `Seq.toDF` gives non-null elements, which is
the #4789 hazard.
- `ConvertToLocalRelation` folds only Project, Filter and Limit, so a
Generate over `VALUES` still reaches the native explode.
- Write NULL arrays as bare `NULL`, because `CAST(NULL AS ARRAY<INT>)`
makes the elements nullable again.
- Seven of these behaviors are in no fixture today: L180, L191, L202,
L240, L297, L381 and L436.
- **`explode_disabled.sql` (new, L214):** set
`spark.comet.exec.explode.enabled=false` and assert the fallback reason.
- **`explode.sql` (L227):** add the `explode(m)` fallback.
- Also move the `rand()` query out into its own file. The whole file is
gated `MinSparkVersion: 3.5` for that one query, so today it is skipped
entirely on Spark 3.4.
- **`posexplode.sql` (L100):** add `posexplode(split(s, 'x'))`.
- **`explode_small_batches.sql` (new):** L447, L465, L488, L613, L629, L646,
using `-- ConfigMatrix: spark.comet.batchSize=4,8` and single-file tables.
- **`explode_limit_offset.sql` (new):** L523, L539, L561, L580, using
single-file tables with `LIMIT 4 OFFSET 1` (#5224).
- **`explode_boolean_struct.sql` (new):** L691 (four instances) and L710,
using a 3000-row table generated with `hash()` (#6464).
- **`explode_boolean_struct_batch_100.sql` (new):** L700, with
`spark.comet.batchSize=100`.
Use `INSERT INTO t SELECT /*+ COALESCE(1) */ ...` or `range(0, n, 1, 1)` for
every batch-boundary fixture. `INSERT ... VALUES` writes one file per row and
would never put two rows in one batch.
## Stays in Scala
- **L38 (8 instances)** pins `CometExplodeExec` canonicalization (outer and
non-outer must differ, identical generators must match) and a
`ReusedExchangeExec` over the generator with AQE on. That is plan shape.
- **L717** feeds an exploded boolean into a Scala UDF (#6464), and UDF
registration has no SQL form. The rest of that shape is covered by L710.
## Notes on the Scala tests
- **The non-outer NULL/empty tests never reach the native operator with
those rows** (L125, L136, L263, L323, L334, L406). Spark's
`InferFiltersFromGenerate` adds a non-NULL, non-empty filter, and
`ConvertToLocalRelation` applies it to the `Seq` data at plan time. The
fixtures keep the queries, but the coverage that matters is in the `_outer`
variants.
- **L447** never has two input batches in one partition.
- **Bare `-0.0`.** `explode.sql:142,156` and `posexplode.sql:131` write
`cast(-0.0 as float/double)`, which is `+0.0`. Use `float('-0.0')` /
`double('-0.0')`.
## 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 |
|---|---|---|---|---|
| `CometGenerateExecSuite` | 38 | generator identity preserves exchange
reuse: $generator($input), AQE=$adaptive [8 instances: explode/posexplode x
s.arr/slice(s.arr, 1, 10) x AQE false/true] | keep | |
| `CometGenerateExecSuite` | 100 | posexplode with a computed array from
Parquet | convert | array/posexplode.sql |
| `CometGenerateExecSuite` | 114 | explode with simple array | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 125 | explode with empty array | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 136 | explode with null array | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 147 | explode_outer with simple array | convert
| array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 158 | explode_outer with empty array | convert
| array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 169 | explode_outer with null array | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 180 | explode with multiple columns | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 191 | explode with array of strings | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 202 | explode with filter | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 214 | explode fallback when disabled | convert
| array/explode_disabled.sql (new) |
| `CometGenerateExecSuite` | 227 | explode with map input falls back |
convert | array/explode.sql |
| `CometGenerateExecSuite` | 240 | explode with nullable projected column |
convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 251 | explode_outer with nullable projected
column | convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 263 | explode with mixed null, empty, and
non-empty arrays | convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 280 | explode_outer with mixed null, empty, and
non-empty arrays | convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 297 | explode with multiple nullable columns |
convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 312 | posexplode with simple array | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 323 | posexplode with empty array | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 334 | posexplode with null array | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 345 | posexplode_outer with simple array |
convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 356 | posexplode with array of strings |
convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 367 | posexplode with nullable elements |
convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 381 | posexplode with multiple projected
columns | convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 393 | posexplode with map input falls back |
covered | array/posexplode.sql |
| `CometGenerateExecSuite` | 406 | posexplode with array of structs |
convert | array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 421 | posexplode in lateral view | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 436 | posexplode of literal array | convert |
array/explode_local_table_scan.sql (new) |
| `CometGenerateExecSuite` | 447 | posexplode across batch boundary with
small batch size | convert | array/explode_small_batches.sql (new) |
| `CometGenerateExecSuite` | 465 | explode_outer across batch boundary with
mixed empty/null rows | convert | array/explode_small_batches.sql (new) |
| `CometGenerateExecSuite` | 488 | posexplode_outer across batch boundary
with mixed empty/null rows | convert | array/explode_small_batches.sql (new) |
| `CometGenerateExecSuite` | 523 | posexplode over limit with offset |
convert | array/explode_limit_offset.sql (new) |
| `CometGenerateExecSuite` | 539 | posexplode_outer over limit with offset |
convert | array/explode_limit_offset.sql (new) |
| `CometGenerateExecSuite` | 561 | explode over limit with offset | convert
| array/explode_limit_offset.sql (new) |
| `CometGenerateExecSuite` | 580 | explode_outer over limit with offset |
convert | array/explode_limit_offset.sql (new) |
| `CometGenerateExecSuite` | 613 | posexplode single row exceeds batch size
| convert | array/explode_small_batches.sql (new) |
| `CometGenerateExecSuite` | 629 | posexplode_outer single row exceeds batch
size | convert | array/explode_small_batches.sql (new) |
| `CometGenerateExecSuite` | 646 | explode single row exceeds batch size |
convert | array/explode_small_batches.sql (new) |
| `CometGenerateExecSuite` | 691 | $generator of structs keeps boolean
fields past the first output batch [instances: explode, explode_outer,
posexplode, posexplode_outer] | convert | array/explode_boolean_struct.sql
(new) |
| `CometGenerateExecSuite` | 700 | explode of structs keeps boolean fields
when one row exceeds the batch size | convert |
array/explode_boolean_struct_batch_100.sql (new) |
| `CometGenerateExecSuite` | 710 | named_struct over an exploded boolean
keeps its values | convert | array/explode_boolean_struct.sql (new) |
| `CometGenerateExecSuite` | 717 | boolean ScalaUDF over an exploded boolean
keeps its values | keep | |
</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]
