chore: Fix Scala code warnings

[athlcode]

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.

[andygrove]

Thanks for taking this on, and for the thorough write-up. The per-execution flag scoping is the right call and the rationale in the POM comment is genuinely useful.

I built the branch locally since CI had not run yet (I have approved the workflows now). ./mvnw test-compile -Pspark-3.5 -Pstrict-warnings passes with zero scalac warnings, as described. However the scalafix check that CI runs (scalafix:scalafix -Dscalafix.mode=CHECK -Psemanticdb -Pspark-3.5) fails on NativeConfigSuite.scala. Details inline.

One thing that is not attached to a line in the diff: nothing in CI runs -Pstrict-warnings, so the next PR that widens an Int into a Long metric quietly reintroduces the warning class this PR clears. A compile-only job running ./mvnw -B test-compile -Pspark-3.5 -Pstrict-warnings -DskipTests in pr_build_linux.yml takes under a minute (about 40s locally) and would sit naturally next to the scalafix job. If you would rather keep that separate, could you open a tracking issue and link it here before this closes #2255?

Everything else checked out: the mapStatus narrowing only affects readers in the same package, all castTimestampTest callers already pass assertNative, and the removed helpers had no callers.

[andygrove]

Running the scalafix check from CI fails on this file. The RedundantSyntax rule (present in both .scalafix.conf and .scalafix-syntactic.conf) wants the s prefix removed since there is no interpolation, so both the syntactic job and the per-profile lint job will go red. Worse, the rewrite it proposes ("$${...}" with no prefix) changes the value to a literal double dollar, so it cannot just be applied.

Could we build these strings with a small helper like "${" + key + "}" instead? That keeps the intent visible and satisfies both the missing-interpolator lint and scalafix. It is also worth running make format on the branch in case spotless has anything to add.

[athlcode]

Replaced the s"$${...}" strings with a small varRef(key) helper ("${" + key + "}")

[andygrove]

Could the Scala 2.13 remainder be a filed issue linked from here rather than "left for a follow-up"? Otherwise the profile stays half-usable with nothing tracking it.

[andygrove]

The specific counts in this comment (~90-120 parameters, ~1,250-1,450 warnings, all but 30 of ~1,300 call sites) will drift as soon as the code changes. I would keep the reasoning and drop the numbers.

[athlcode]

dropped the numbers

[andygrove]

Did you consider keeping params on and silencing the known sites with -Wconf filters, e.g. -Wconf:cat=unused-params&site=org\.apache\.comet\.Native.*:s plus the shim packages? -Wconf filters do not trigger the unused-@nowarn lint, so that might avoid dropping the flag for the whole codebase. Happy to hear if you tried it and it did not work out.

[athlcode]

@andygrove
Thanks for the -Wconf pointer. I tried it rather than guessing, and on Scala 2.12.18 it works the way you expected:

-Wconf:cat=unused-params&site=org\.apache\.comet\.Native\..*:s,cat=unused-params&src=.*/src/[a-z]+/spark-[^/]+/.*:s

Those two filters silence Native and every shim source, which is 96 of the 163 unused-parameter warnings on -Pspark-3.5. The other 67 are in shared sources, though, so turning params back on with just these filters still fails the build:

• 27 in main: 4 are on private methods and can simply be removed. The other 23 are on public extension points and serde helpers where the parameter is part of the signature: overridable defaults like getSupportLevel and CometScanContrib.tryTransformV1, and helpers like createBinaryExpr(expr, …) (13 callers).
• 40 in tests: 4 are genuinely unused and can be removed. The other 36 are in fixtures with fixed signatures, almost all of them fakes of Celeborn's client API.

I can see two ways forward and would like your preference before changing anything:

A. Keep params out of the profile, as the PR does now.

B. Turn params back on with the Native and shim filters, remove the 8 unused parameters, and add per-method -Wconf site filters for the remaining 59. New code keeps the check, but the POM carries a longer filter list that has to be updated whenever a signature like that is added.

For this PR I'd lean towards A, plus a follow-up issue for B. That follow-up could also drop the unused expr parameter from the serde helpers, which is an API change I didn't want to fold in here. If you'd rather have B land now, I'm happy to do it. Which do you prefer?

[andygrove]

This change does what the TODO above asks for (I confirmed the long overload exists in parquet-hadoop 1.13.1, which the 3.4 and 3.5 profiles use), so the TODO can go.

[athlcode]

Removed, thanks for checking the 1.13.1 overload.

[andygrove]

IcebergReflection.getTasks has a single caller, which is this one. Would returning Option[java.util.List[AnyRef]] from getTasks let us drop the cast and the comment here? The cast would then live with the other reflection casts.

[athlcode]

Done. getTasks and its helpers return Option[java.util.List[AnyRef]], so the cast now lives with the other reflection casts.

[andygrove]

Does deleteBuilder.addAllEqualityIds(equalityIds) work here? It avoids the discarded value entirely.

[athlcode]

Yes, switched to it.

[sunchao]

Correctness

This makes the opt-in strict-warnings profile usable for Scala 2.12 by separating main and test compiler arguments, removing unused code and making conversions and discarded results explicit. I reviewed all 106 changed files against 3810936b.

The runtime edits preserve the relevant Spark 3.5/4.0 contracts I checked: byte/short literals retain their signed values, metrics still receive Long values, and the broadcast callers now supply Any explicitly to Spark's generic executeBroadcast API. The shuffle visibility changes keep the actual readers within the permitted package. The Parquet test helper now uses the existing long overload, avoiding its previous narrowing conversion. Removed private helpers have no callers, and callers of the helpers losing default arguments already supply those arguments.

One existing P2 remains: the NativeConfigSuite/RedundantSyntax conflict. I reproduced it with the cached scalafix 0.10.4 engine used by the Maven lint plugin: the complete base file passes, while this head fails and proposes seven prefix-only rewrites that leave doubled dollar signs in the resulting strings. Please address that thread before merging. I am not adding a duplicate inline comment.

At 22:13 UTC on September 12, CI has 54 successful, 10 skipped and 6 failed checks, including the four profile lint jobs, syntactic Scala lint and Required Checks. I verified all five lint logs at merge 5ab7e100cbefbcbb97912dcb15fd984df4dd2a71, whose tree matches this head: they show the same NativeConfigSuite rewrite conflict. The syntactic lint log gives the exact diff; the offline reproduction above independently confirms the Maven lint engine's behavior. This does not establish complete runtime test coverage or a clean strict-profile build. The reported clean Spark 3.5 strict compilation remains author/maintainer evidence. Maintained Spark 3.4/4.1 sources were unavailable, and I did not run Spark or native product suites locally.

Performance

The explicit numeric widening and discarded-result bindings do not introduce extra collection passes or copies in the inspected paths. Moving LocalShuffleOutput to the companion removes its unnecessary outer-instance association while retaining the same per-write fields. Benchmark edits primarily make row counts explicit Long values and keep the existing workloads. This PR adds no new expression or measured performance claim, so I have no additional microbenchmark request.

Design

Per-execution compiler configuration is a sensible way to retain value-discard warnings in main code while accommodating test APIs that return assertion or plan objects. The profile remains opt-in, and the POM documents the remaining Scala 2.13 limitations. The existing review already asks for CI enforcement and tracking of the remaining work; those points should be closed out in that discussion.

Abstraction & complexity

The change stays within the existing compiler profile and execution classes. The companion-level shuffle output record is a small simplification, and the reflection casts preserve the same erased Java types. Removing unused helpers reduces code without removing their active alternatives. I found no additional abstraction or complexity issue requiring a separate finding.

[sunchao]

Rechecked 33f16426. The earlier P2 Scalafix conflict is fixed by varRef: cached Scalafix 0.10.4 passes the updated file and current base, while the prior head still reproduces all seven rewrites. A bounded JDK 17/Scala 2.13.10 check also confirms the helper retains the exact single-dollar reference strings.

The reflection typing cleanup, bulk equality-ID call, rebased changes and new Spark 3.5 strict-warning compile job introduce no further findings. The local CI configuration check passes. Current product workflows are awaiting approval (action_required), so full build and test results are still pending.

The unused-parameter check remains excluded from both main and test compilation. The proposed narrower -Wconf filtering would add lint coverage; whether to include that work here remains an open scope decision in that discussion.

[andygrove]

The new job comment says the Scala 2.13 remainder is tracked separately, but I could not find an issue for it. Searching open and closed issues turns up only #2255, which this PR closes. The POM comment it points at also just says the 2.13 warnings are left for a follow-up. That leaves the profile usable on 2.12 only, CI enforcing one profile, and the remaining 2.13 warnings with nothing recording them. Could you open an issue for the -Xlint:nonlocal-return and non-exhaustive-match work and link it from both this comment and the POM?

On the POM comment, I think we still have the drift problem I raised earlier. The value-discard counts are gone, but the unused-params paragraph still says ~90-120 parameters per profile and 64 @native declarations, and those were two of the numbers I was worried about. Both go stale the moment someone adds a @native declaration or a shim. Could we drop them and keep just the reasoning about Native.scala and the cross-version shims?

Separately, I went looking for a silent behavior change behind any of these warning fixes and did not find one, which is the main risk with a change this mechanical. The private[shuffle] additions still leave public classes and constructors in bytecode so Spark's reflective spark.shuffle.manager instantiation resolves, every val _ = site sits in a method already declared Unit, the removed default arguments are supplied explicitly at every call site, and the removed helpers have no remaining references.

[andygrove]

One more thing I missed on the first pass. The new strict-scala-warnings job goes straight to ./mvnw with no Bootstrap Maven step. Every other job in pr_build_linux.yml that calls ./mvnw directly now uses ./.github/actions/maven-bootstrap first, added by #5852, and the workflows README says any job whose first Maven use is a bare ./mvnw needs it. Without it a blip fetching the Maven distribution fails the job before anything compiles, and since this rolls up into Required Checks that evicts the PR from the merge queue. Could you add the step between the cache restore and the compile?

[athlcode]

One more thing I missed on the first pass. The new strict-scala-warnings job goes straight to ./mvnw with no Bootstrap Maven step. Every other job in pr_build_linux.yml that calls ./mvnw directly now uses ./.github/actions/maven-bootstrap first, added by #5852, and the workflows README says any job whose first Maven use is a bare ./mvnw needs it. Without it a blip fetching the Maven distribution fails the job before anything compiles, and since this rolls up into Required Checks that evicts the PR from the merge queue. Could you add the step between the cache restore and the compile?

@andygrove sorry I forgot to push those changes, have pushed it now, please do review, thank you

[athlcode]

The new job comment says the Scala 2.13 remainder is tracked separately, but I could not find an issue for it. Searching open and closed issues turns up only #2255, which this PR closes. The POM comment it points at also just says the 2.13 warnings are left for a follow-up. That leaves the profile usable on 2.12 only, CI enforcing one profile, and the remaining 2.13 warnings with nothing recording them. Could you open an issue for the -Xlint:nonlocal-return and non-exhaustive-match work and link it from both this comment and the POM?

On the POM comment, I think we still have the drift problem I raised earlier. The value-discard counts are gone, but the unused-params paragraph still says ~90-120 parameters per profile and 64 @native declarations, and those were two of the numbers I was worried about. Both go stale the moment someone adds a @native declaration or a shim. Could we drop them and keep just the reasoning about Native.scala and the cross-version shims?

Separately, I went looking for a silent behavior change behind any of these warning fixes and did not find one, which is the main risk with a change this mechanical. The private[shuffle] additions still leave public classes and constructors in bytecode so Spark's reflective spark.shuffle.manager instantiation resolves, every val _ = site sits in a method already declared Unit, the removed default arguments are supplied explicitly at every call site, and the removed helpers have no remaining references.

@andygrove I have opened an issue for the -Xlint:nonlocal-return and non-exhaustive-match work and link it from both this comment and the POM: #5893
please let me know if I need to add any more information in the issue or you can add as well thank you

Forgot to push latest changes, I have pushed the changes to drop the ~90-120 parameters per profile and 64 @native declarations now, please do review and thank you

[rich7420]

@athlcode thanks for the patch