andygrove opened a new issue, #5569:
URL: https://github.com/apache/datafusion-comet/issues/5569
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").
--
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]
