athlcode opened a new pull request, #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876
## Which issue does this PR close?
Closes #2255.
## Rationale for this change
#2254 added the `strict-warnings` profile but nothing yet builds cleanly
under it. Running it as the issue describes reports **1,993 warnings** on
`-Pspark-3.5` (197 in `src/main`, 1,796 in `src/test`), so the profile can't be
used as written and can't be wired into CI.
Two things worth knowing for anyone reproducing this:
- scalac caps output at 100 warnings per compilation unit, so an unmodified
run reports exactly 200 (100 main + 100 test) and buries `100 warnings found`
in the log. You need `-Xmaxwarns` to see the real total — and the syntax
differs by Scala version: `-Xmaxwarns:N` is 2.13-only and hard-fails 2.12 with
`'-Xmaxwarns' does not accept multiple arguments`.
- The warning sets barely overlap between Scala 2.12 and 2.13, so a run
against one profile says little about the other.
## What changes are included in this PR?
106 files (+482/−426): `pom.xml`, 37 main sources, 68 test sources.
### 1. Scope the profile's flags (`pom.xml`)
`args` is now configured per execution rather than on the plugin, so
`src/main` and `src/test` can differ. Two lints are deliberately absent, with
the reasoning recorded in a comment above the profile:
- **`-Ywarn-unused:params`** (163 warnings) — dropped from both. 64 are
`Native.scala`, which is nothing but `@native` declarations whose parameters
have no body to be used in; most of the rest are cross-version shims that take
a parameter to satisfy the Spark version they shim. These can't be annotated
away individually either: the set differs between 2.12 and 2.13
(`CometScanContrib.scala` warns under spark-3.5 but not spark-4.0, and vice
versa for `ShimSparkErrorConverter.scala`), so any `@nowarn` that silences one
profile is an *unused annotation* on the other — which `-Xlint:_` reports via
`-Xlint:nowarn` and `-Xfatal-warnings` turns into a failure.
- **`-Ywarn-value-discard`** (1,528 test warnings) — kept for `src/main`,
dropped for `src/test`. In a ScalaTest suite the two largest groups are the
idiom itself: a trailing `assert(...)` discards an `org.scalatest.Assertion`,
and `checkSparkAnswerAndOperator` discards the `(SparkPlan, SparkPlan)` it
returns at all but 30 of its ~1,300 call sites. It stays on for main, where a
discarded result is usually a dropped builder or a swallowed return.
### 2. Fix the remaining 302 warnings in source
| Category | Count |
|---|---|
| implicit numeric widening | 226 |
| discarded non-Unit value (main) | 32 |
| unused private / local / pattern var | 16 |
| public member exposing a `private[spark]` type | 9 |
| possible missing interpolator | 7 |
| dead code following this construct | 4 |
| inferred existential type | 2 |
| ineffective `@nowarn` | 2 |
| deprecated API, inferred `Any`, uncheckable outer reference, adapted arg
list | 4 |
Most are mechanical (`.toLong`, `val _ =`), but a few are worth a reviewer's
eye:
- **`CometTestBase.makeParquetFile`** used
`.withRowGroupSize(rowGroupSize.toInt)`, selecting parquet's deprecated `int`
overload and silently truncating a `Long` row-group size. Now calls the `long`
overload.
- **`SpillSorterSuite`** had `allocateArray(INITIAL_SIZE * 2)` — an `Int`
multiply widened to `Long` afterwards, which is exactly the overflow class this
lint exists to catch.
- **`CometColumnarToRowExec` / `CometNativeColumnarToRowExec`** called
`child.executeBroadcast()` with no type argument, inferring
`Broadcast[Nothing]` and making the following line dead code. Now
`executeBroadcast[Any]()`.
- **`CometNativeShuffleWriter.mapStatus`** was a public `var` exposing the
`private[spark]` `MapStatus`; narrowed to `private[shuffle]`, which is all the
tests that read it need.
- **`LocalShuffleOutput`** moved from an inner case class to the companion
object, so type tests against it no longer carry an outer reference that can't
be checked at run time.
- **`NativeConfigSuite`** had literal `${...}` strings used to test Hadoop
variable substitution; rewritten as `s"$${...}"` so the intent is explicit and
the value is unchanged.
- Dead code removed: `CometScanRule.isDynamicPruningFilter`,
`CometNativeCastSuite.castFallbackTest`,
`CometPlanStabilitySuite.getSimplifiedPlan` (and the imports and
`referenceRegex` it orphaned).
- `@nowarn` is used only where a signature genuinely can't change — the four
`ShuffleManager` SPI overrides — with a message filter rather than a blanket
suppression.
## How are these changes tested?
No new tests: this is a build-hygiene change with no intended behaviour
change, and the touched code is covered by the existing suites.
Verified by compiling with the profile enabled:
```
./mvnw clean test-compile -Pspark-3.5 -Pstrict-warnings → BUILD SUCCESS,
0 errors
```
`-Pspark-4.0` also compiles, and its warning count drops from 231 (main) to
83, but **Scala 2.13 is not yet clean** — 100 remain (83 main, 17 test),
dominated by two categories that 2.12 does not raise at all:
- **42 `-Xlint:nonlocal-return`** — a `return` inside a closure, which the
compiler implements by throwing (17 of them in `CometIcebergNativeWrite.scala`).
- **20 non-exhaustive matches.**
Clearing those means restructuring control flow rather than annotating it,
across the Iceberg write path, cast support and shuffle — a separate change
with a real behaviour-risk profile, so it is left for a follow-up rather than
silenced here. That caveat is also recorded in the POM comment so the profile
doesn't read as passing everywhere.
--
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]
