andygrove opened a new issue, #6164: URL: https://github.com/apache/datafusion-comet/issues/6164
#5876 made `-Pstrict-warnings` pass on the Scala 2.12 profiles, but it left `-Ywarn-unused:params` out of both the main and the test flag lists. Most of what that lint reports are parameters a signature requires, not dead ones. That covers the `@native` declarations in `Native.scala`, the cross-version shims under `src/main/spark-*`, overridable defaults and serde helpers, and test fakes with fixed signatures. `@nowarn` can't silence them one at a time, because the set differs between Scala 2.12 and 2.13, so an annotation one profile needs is flagged as unused on the other. `-Wconf` filters don't have that problem. In the #5876 thread, @athlcode [measured](https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998842774) that these two filters silence `Native` and every shim source on Scala 2.12.18: ``` -Wconf:cat=unused-params&site=org\.apache\.comet\.Native\..*:s,cat=unused-params&src=.*/src/[a-z]+/spark-[^/]+/.*:s ``` On `-Pspark-3.5` at the time, that covered 96 of the 163 unused-parameter warnings. Of the other 67, eight are genuinely unused (four on private methods in main, four in tests) and can simply be removed. The remaining 59 fall into three groups: - Public extension points such as `getSupportLevel` and `CometScanContrib.tryTransformV1`. - Serde helpers such as `QueryPlanSerde.createBinaryExpr`, whose `expr` parameter is never read even though all 13 callers pass it. - Test fixtures, almost all of them fakes of Celeborn's client API. The work: - Turn `-Ywarn-unused:params` back on in both executions of the profile, with the two filters above. - Remove the parameters that are genuinely unused. - Drop the unused `expr` parameter from `createBinaryExpr` and any other serde helper that carries it, and update the callers. - Cover what is left with per-site `-Wconf` filters, or change the signature where that is cleaner. - Rewrite the `-Ywarn-unused:params` paragraph of the POM comment to match. While there, fix the flag it names. It says unused `@nowarn` annotations are reported "via `-Xlint:nowarn`", but that is not a valid `-Xlint` choice on either 2.12 or 2.13. The warning comes from `-Xlint:unused`, which turns on `-Ywarn-unused:nowarn`. One unrelated cleanup from the same review can go in the same PR. The `strict-scala-warnings` job in `pr_build_linux.yml` has its own `Bootstrap Maven` step. But since #5881, `setup-builder` runs `maven-bootstrap` as its last step, so the extra step just repeats it. The Scala 2.13 profiles are tracked separately in #5893. Done when `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings -DskipTests` passes with `-Ywarn-unused:params` enabled for both main and test sources. -- 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]
