apache / apache/datafusion-comet

Remove stale dev/diffs test exclusions whose tracking issues are closed

Open
#5,569 1 comment 0 reactions 1 assignee Claimed by @rich7420 View on GitHub
area:ci enhancement priority:medium test
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

Three exclusions in `dev/diffs/*.diff` cite Comet issues that have since been fixed, so they now suppress Spark SQL test coverage for no reason. Each is present in more than one version diff, so they should be removed across all affected diffs in one pass rather than in whichever version PR happens to touch them next: new version diffs are seeded from the previous version's, so a partial fix diverges the diffs and the next reseed reintroduces the stale entry.

Surfaced in review of #4950 by @sunchao.

### 1. Variant shredding suites

`VariantShreddingSuite` and `ParquetVariantShreddingSuite` are given an `IgnoreCometSuite` mixin, which skips all 15 tests when `ENABLE_COMET=true`.

- Cited issue: #2209, closed by merged PR #4084.
- #4084 added the shredded-Variant scan fallback and **already removed these same exclusions from `dev/diffs/4.0.4.diff`** (it has zero occurrences of `VariantShreddingSuite`); `4.1.3.diff` was never cleaned up.
- Affected: `dev/diffs/4.1.3.diff`, `dev/diffs/4.2.0.diff`.
- Removing the mixins should also drop the now-unused `IgnoreCometSuite` import in each file.

This is the highest-value one: it drops regression coverage for the silent-null corruption that #4084 fixed.

### 2. `having-and-order-by-recursive-type-name-resolution.sql`

The input file carries a `--SET spark.comet.enabled = false` header, which runs all 45 queries without Comet.

- Cited issue: #4123, closed by merged PR #4157, which added recursive fallback for map-containing aggregate keys plus a regression test for the exact `Struct(Map(...))` HAVING / ORDER BY shape.
- Affected: `dev/diffs/4.1.3.diff`, `dev/diffs/4.2.0.diff`.

### 3. Parquet V2 writer matrix in `ParquetTypeWideningSuite`

`withAllParquetWriters { ... }` is commented out, leaving all 82 generated widening cases on Parquet's default V1 writer.

- Cited issue: #574, closed: the active `native_datafusion` and `native_iceberg_compat` scans support `DELTA_BINARY_PACKED`, `DELTA_BYTE_ARRAY` and `DELTA_LENGTH_BYTE_ARRAY`.
- The related `SPARK-40128 read DELTA_LENGTH_BYTE_ARRAY encoded strings` test in `ParquetIOSuite` is still tagged `IgnoreComet("Comet doesn't support DELTA encoding yet")` and should be re-enabled with it.
- Affected: `dev/diffs/4.0.4.diff`, `dev/diffs/4.1.3.diff`, `dev/diffs/4.2.0.diff` (byte-identical in all three).
- If the legacy `native_comet` scan still cannot read these encodings, scope the exclusion to that scan implementation rather than removing the whole matrix.

### Suggested approach

One PR per item, each removing the exclusion from every diff that carries it, verified by the corresponding `Spark SQL Tests` CI job for each affected version. Where a specific case still fails, replace the blanket exclusion with a case-scoped `IgnoreComet` linked to a *current* issue, per the rule in `docs/source/contributor-guide/adding_a_new_spark_version.md` ("do not disable whole suites").

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.