You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "athlcode (via GitHub)" <gi...@apache.org> on 2026/09/12 11:47:00 UTC
[PR] chore: Fix Scala code warnings [datafusion-comet]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998815693
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -353,7 +353,7 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
val equalityIds = equalityIdsMethod
.invoke(deleteFile)
.asInstanceOf[java.util.List[Integer]]
- equalityIds.forEach(id => deleteBuilder.addEqualityIds(id))
+ equalityIds.forEach(id => { val _ = deleteBuilder.addEqualityIds(id) })
Review Comment:
Done. It sits mid-block, so nothing is discarded.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#issuecomment-5778665184
@andygrove Please review when you get a chance, thank you
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998814944
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -794,7 +794,7 @@ abstract class CometTestBase
.withType(schema)
// TODO we need to shim this and use withRowGroupSize(Long) with later parquet-hadoop versions to remove
// the deprecated warning here
- .withRowGroupSize(rowGroupSize.toInt)
+ .withRowGroupSize(rowGroupSize)
Review Comment:
Removed, thanks for checking the 1.13.1 overload.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r4083712941
##########
pom.xml:
##########
@@ -859,29 +859,85 @@ under the License.
</pluginManagement>
</build>
</profile>
+ <!--
+ Compile Scala with warnings promoted to errors. Not active by default; run it
+ explicitly, e.g. `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings`.
+
+ This passes on the Scala 2.12 profiles. The 2.13 profiles (spark-4.0 and later)
+ still report warnings that 2.12 does not raise at all, dominated by
+ `-Xlint:nonlocal-return` (a `return` inside a closure, which the compiler
+ implements by throwing) and non-exhaustive matches. Clearing those means
+ restructuring control flow rather than annotating it, so they are left for a
+ follow-up rather than silenced here.
+
+ `args` is configured per execution rather than on the plugin, because main and
+ test sources warrant different flags (see `-Ywarn-value-discard` below). An
+ execution's `args` replaces the plugin-level list instead of appending to it, so
+ each list below is self-contained.
+
+ Two lints are deliberately absent from both lists:
+
+ `-Ywarn-unused:params` reports ~90-120 parameters per profile, and essentially
+ all of them are structurally unfixable rather than dead: `Native.scala` is 64
+ `@native` declarations whose parameters have no body to be used in, and the rest
+ are cross-version shims under `src/main/spark-3.x`, `spark-4.x` and friends that
+ take a parameter to satisfy the signature of the Spark version they shim. Nor can
+ those be annotated away one by one: the set differs between Scala 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:_` then reports via
+ `-Xlint:nowarn` and `-Xfatal-warnings` turns into a build failure.
+
+ `-Ywarn-value-discard` stays on for main sources, where a discarded result is
+ usually a dropped builder or a swallowed return, but is off for test sources.
+ Under it a ScalaTest suite reports ~1,250-1,450 warnings, and the two largest
+ groups are the idiom itself: an `assert(...)` in trailing position discards an
+ `org.scalatest.Assertion`, and `checkSparkAnswerAndOperator` discards the
+ `(SparkPlan, SparkPlan)` it returns at all but 30 of its ~1,300 call sites.
+ -->
<profile>
- <id>strict-warnings</id>
- <build>
- <plugins>
- <plugin>
- <groupId>net.alchim31.maven</groupId>
- <artifactId>scala-maven-plugin</artifactId>
- <configuration>
- <args>
- <arg>-deprecation</arg>
- <arg>-unchecked</arg>
- <arg>-feature</arg>
- <arg>-Xlint:_</arg>
- <arg>-Ywarn-dead-code</arg>
- <arg>-Ywarn-numeric-widen</arg>
- <arg>-Ywarn-value-discard</arg>
- <arg>-Ywarn-unused:imports,patvars,privates,locals,params,-implicits</arg>
- <arg>-Xfatal-warnings</arg>
- </args>
- </configuration>
- </plugin>
- </plugins>
- </build>
+ <id>strict-warnings</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>net.alchim31.maven</groupId>
+ <artifactId>scala-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>scala-compile-first</id>
+ <configuration>
+ <args>
+ <arg>-deprecation</arg>
+ <arg>-unchecked</arg>
+ <arg>-feature</arg>
+ <arg>-Xlint:_</arg>
+ <arg>-Ywarn-dead-code</arg>
+ <arg>-Ywarn-numeric-widen</arg>
+ <arg>-Ywarn-value-discard</arg>
+ <arg>-Ywarn-unused:imports,patvars,privates,locals,-implicits</arg>
Review Comment:
Sorry for the slow reply, and thanks for actually trying the `-Wconf` filters. Let's go with A for this PR. Could you file a follow-up issue for B, including dropping the unused `expr` parameter from the serde helpers, and link it from the `-Ywarn-unused:params` paragraph in the POM comment the way #5893 is linked?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998814690
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -486,11 +486,17 @@ case class CometScanRule(session: SparkSession)
// which bucket's per-bucket S3 settings the single native FileIO consumes (see
// catalogProperties below), so this must run before we build them. The task list is
// handed to `extract` below so the reflective accessor runs once per scan.
+ // The task list is cast to `java.util.List[AnyRef]` rather than left at the
+ // `java.util.List[_]` that `getTasks` returns: a wildcard element type makes this
+ // pair infer a top-level existential, which `-Xlint:existential` rejects. The list
+ // is erased at runtime, so the cast costs nothing.
val (icebergTasks, taskValidation) =
try {
IcebergReflection.getTasks(scanExec.scan) match {
case Some(tasks) =>
- (tasks, CometScanRule.validateIcebergFileScanTasks(tasks, s3CompliantSchemes))
+ (
+ tasks.asInstanceOf[java.util.List[AnyRef]],
Review Comment:
Done in getTasks and its two private helpers, the cast and comment in CometScanRule are gone.
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -794,7 +794,7 @@ abstract class CometTestBase
.withType(schema)
// TODO we need to shim this and use withRowGroupSize(Long) with later parquet-hadoop versions to remove
// the deprecated warning here
- .withRowGroupSize(rowGroupSize.toInt)
+ .withRowGroupSize(rowGroupSize)
Review Comment:
done
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#issuecomment-5799711302
> Thanks for keeping this current with main. I ran `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings -DskipTests` on `eaa3f8cfb` since CI hasn't run on it yet. It fails with 10 errors, all in test code that landed on main after the last green run. `-Pspark-3.4` gives the same 10.
>
> * `SerdeRegistrationSuite.scala:33-36` warns about an adapted argument list. It needs double parentheses, e.g. `"math" -> ((baseMathExpressions, sparkVersionSpecificMathExpressions))`.
> * `CometUnixTimestampBenchmark.scala:37,51,60`, `CometParquetWriterSuite.scala:1755`, `CometBinaryLengthBenchmark.scala:60` and `CometIcebergWriteBenchmark.scala:331` are implicit `Int` to `Long` widenings.
>
> Could you merge main again, fix these, and run the strict compile locally before pushing? Once that head is up I'll approve the workflow run so the new job gets a real result.
@andygrove fixed these, please review, thank you
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998842774
##########
pom.xml:
##########
@@ -859,29 +859,85 @@ under the License.
</pluginManagement>
</build>
</profile>
+ <!--
+ Compile Scala with warnings promoted to errors. Not active by default; run it
+ explicitly, e.g. `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings`.
+
+ This passes on the Scala 2.12 profiles. The 2.13 profiles (spark-4.0 and later)
+ still report warnings that 2.12 does not raise at all, dominated by
+ `-Xlint:nonlocal-return` (a `return` inside a closure, which the compiler
+ implements by throwing) and non-exhaustive matches. Clearing those means
+ restructuring control flow rather than annotating it, so they are left for a
+ follow-up rather than silenced here.
+
+ `args` is configured per execution rather than on the plugin, because main and
+ test sources warrant different flags (see `-Ywarn-value-discard` below). An
+ execution's `args` replaces the plugin-level list instead of appending to it, so
+ each list below is self-contained.
+
+ Two lints are deliberately absent from both lists:
+
+ `-Ywarn-unused:params` reports ~90-120 parameters per profile, and essentially
+ all of them are structurally unfixable rather than dead: `Native.scala` is 64
+ `@native` declarations whose parameters have no body to be used in, and the rest
+ are cross-version shims under `src/main/spark-3.x`, `spark-4.x` and friends that
+ take a parameter to satisfy the signature of the Spark version they shim. Nor can
+ those be annotated away one by one: the set differs between Scala 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:_` then reports via
+ `-Xlint:nowarn` and `-Xfatal-warnings` turns into a build failure.
+
+ `-Ywarn-value-discard` stays on for main sources, where a discarded result is
+ usually a dropped builder or a swallowed return, but is off for test sources.
+ Under it a ScalaTest suite reports ~1,250-1,450 warnings, and the two largest
+ groups are the idiom itself: an `assert(...)` in trailing position discards an
+ `org.scalatest.Assertion`, and `checkSparkAnswerAndOperator` discards the
+ `(SparkPlan, SparkPlan)` it returns at all but 30 of its ~1,300 call sites.
+ -->
<profile>
- <id>strict-warnings</id>
- <build>
- <plugins>
- <plugin>
- <groupId>net.alchim31.maven</groupId>
- <artifactId>scala-maven-plugin</artifactId>
- <configuration>
- <args>
- <arg>-deprecation</arg>
- <arg>-unchecked</arg>
- <arg>-feature</arg>
- <arg>-Xlint:_</arg>
- <arg>-Ywarn-dead-code</arg>
- <arg>-Ywarn-numeric-widen</arg>
- <arg>-Ywarn-value-discard</arg>
- <arg>-Ywarn-unused:imports,patvars,privates,locals,params,-implicits</arg>
- <arg>-Xfatal-warnings</arg>
- </args>
- </configuration>
- </plugin>
- </plugins>
- </build>
+ <id>strict-warnings</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>net.alchim31.maven</groupId>
+ <artifactId>scala-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>scala-compile-first</id>
+ <configuration>
+ <args>
+ <arg>-deprecation</arg>
+ <arg>-unchecked</arg>
+ <arg>-feature</arg>
+ <arg>-Xlint:_</arg>
+ <arg>-Ywarn-dead-code</arg>
+ <arg>-Ywarn-numeric-widen</arg>
+ <arg>-Ywarn-value-discard</arg>
+ <arg>-Ywarn-unused:imports,patvars,privates,locals,-implicits</arg>
Review Comment:
@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?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#issuecomment-5796915562
Thanks for keeping this current with main. I ran `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings -DskipTests` on `eaa3f8cfb` since CI hasn't run on it yet. It fails with 10 errors, all in test code that landed on main after the last green run. `-Pspark-3.4` gives the same 10.
- `SerdeRegistrationSuite.scala:33-36` warns about an adapted argument list. It needs double parentheses, e.g. `"math" -> ((baseMathExpressions, sparkVersionSpecificMathExpressions))`.
- `CometUnixTimestampBenchmark.scala:37,51,60`, `CometParquetWriterSuite.scala:1755`, `CometBinaryLengthBenchmark.scala:60` and `CometIcebergWriteBenchmark.scala:331` are implicit `Int` to `Long` widenings.
Could you merge main again, fix these, and run the strict compile locally before pushing? Once that head is up I'll approve the workflow run so the new job gets a real result.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998811557
##########
pom.xml:
##########
@@ -859,29 +859,85 @@ under the License.
</pluginManagement>
</build>
</profile>
+ <!--
+ Compile Scala with warnings promoted to errors. Not active by default; run it
+ explicitly, e.g. `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings`.
+
+ This passes on the Scala 2.12 profiles. The 2.13 profiles (spark-4.0 and later)
+ still report warnings that 2.12 does not raise at all, dominated by
+ `-Xlint:nonlocal-return` (a `return` inside a closure, which the compiler
+ implements by throwing) and non-exhaustive matches. Clearing those means
+ restructuring control flow rather than annotating it, so they are left for a
+ follow-up rather than silenced here.
+
+ `args` is configured per execution rather than on the plugin, because main and
+ test sources warrant different flags (see `-Ywarn-value-discard` below). An
+ execution's `args` replaces the plugin-level list instead of appending to it, so
+ each list below is self-contained.
+
+ Two lints are deliberately absent from both lists:
+
+ `-Ywarn-unused:params` reports ~90-120 parameters per profile, and essentially
Review Comment:
dropped the numbers
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#issuecomment-5796917182
Now that `Strict Scala warnings` gates the PR tier and the queue, could we add a step to the "Submitting a Pull Request" checklist in `docs/source/contributor-guide/development.md` next to the Clippy step? The default build profile is Spark 4.1 on Scala 2.13, which doesn't raise most of these warnings. So someone who only builds the default profile will get a red job with no hint of how to reproduce it. Something like `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings -DskipTests` with a sentence explaining why it's the 3.5 profile would cover it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998815693
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -353,7 +353,7 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
val equalityIds = equalityIdsMethod
.invoke(deleteFile)
.asInstanceOf[java.util.List[Integer]]
- equalityIds.forEach(id => deleteBuilder.addEqualityIds(id))
+ equalityIds.forEach(id => { val _ = deleteBuilder.addEqualityIds(id) })
Review Comment:
Yes, switched to it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998813057
##########
spark/src/test/scala/org/apache/comet/objectstore/NativeConfigSuite.scala:
##########
@@ -108,33 +108,33 @@ class NativeConfigSuite extends AnyFunSuite with Matchers {
}
}
- test("extractObjectStoreOptions - forwards the substituted value of a ${...} reference") {
+ test(s"extractObjectStoreOptions - forwards the substituted value of a $${...} reference") {
Review Comment:
Replaced the s"$${...}" strings with a small varRef(key) helper ("${" + key + "}")
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3997031038
##########
pom.xml:
##########
@@ -859,29 +859,85 @@ under the License.
</pluginManagement>
</build>
</profile>
+ <!--
+ Compile Scala with warnings promoted to errors. Not active by default; run it
+ explicitly, e.g. `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings`.
+
+ This passes on the Scala 2.12 profiles. The 2.13 profiles (spark-4.0 and later)
+ still report warnings that 2.12 does not raise at all, dominated by
+ `-Xlint:nonlocal-return` (a `return` inside a closure, which the compiler
+ implements by throwing) and non-exhaustive matches. Clearing those means
+ restructuring control flow rather than annotating it, so they are left for a
+ follow-up rather than silenced here.
Review Comment:
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.
##########
spark/src/test/scala/org/apache/comet/objectstore/NativeConfigSuite.scala:
##########
@@ -108,33 +108,33 @@ class NativeConfigSuite extends AnyFunSuite with Matchers {
}
}
- test("extractObjectStoreOptions - forwards the substituted value of a ${...} reference") {
+ test(s"extractObjectStoreOptions - forwards the substituted value of a $${...} reference") {
Review Comment:
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.
##########
pom.xml:
##########
@@ -859,29 +859,85 @@ under the License.
</pluginManagement>
</build>
</profile>
+ <!--
+ Compile Scala with warnings promoted to errors. Not active by default; run it
+ explicitly, e.g. `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings`.
+
+ This passes on the Scala 2.12 profiles. The 2.13 profiles (spark-4.0 and later)
+ still report warnings that 2.12 does not raise at all, dominated by
+ `-Xlint:nonlocal-return` (a `return` inside a closure, which the compiler
+ implements by throwing) and non-exhaustive matches. Clearing those means
+ restructuring control flow rather than annotating it, so they are left for a
+ follow-up rather than silenced here.
+
+ `args` is configured per execution rather than on the plugin, because main and
+ test sources warrant different flags (see `-Ywarn-value-discard` below). An
+ execution's `args` replaces the plugin-level list instead of appending to it, so
+ each list below is self-contained.
+
+ Two lints are deliberately absent from both lists:
+
+ `-Ywarn-unused:params` reports ~90-120 parameters per profile, and essentially
Review Comment:
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.
##########
pom.xml:
##########
@@ -859,29 +859,85 @@ under the License.
</pluginManagement>
</build>
</profile>
+ <!--
+ Compile Scala with warnings promoted to errors. Not active by default; run it
+ explicitly, e.g. `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings`.
+
+ This passes on the Scala 2.12 profiles. The 2.13 profiles (spark-4.0 and later)
+ still report warnings that 2.12 does not raise at all, dominated by
+ `-Xlint:nonlocal-return` (a `return` inside a closure, which the compiler
+ implements by throwing) and non-exhaustive matches. Clearing those means
+ restructuring control flow rather than annotating it, so they are left for a
+ follow-up rather than silenced here.
+
+ `args` is configured per execution rather than on the plugin, because main and
+ test sources warrant different flags (see `-Ywarn-value-discard` below). An
+ execution's `args` replaces the plugin-level list instead of appending to it, so
+ each list below is self-contained.
+
+ Two lints are deliberately absent from both lists:
+
+ `-Ywarn-unused:params` reports ~90-120 parameters per profile, and essentially
+ all of them are structurally unfixable rather than dead: `Native.scala` is 64
+ `@native` declarations whose parameters have no body to be used in, and the rest
+ are cross-version shims under `src/main/spark-3.x`, `spark-4.x` and friends that
+ take a parameter to satisfy the signature of the Spark version they shim. Nor can
+ those be annotated away one by one: the set differs between Scala 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:_` then reports via
+ `-Xlint:nowarn` and `-Xfatal-warnings` turns into a build failure.
+
+ `-Ywarn-value-discard` stays on for main sources, where a discarded result is
+ usually a dropped builder or a swallowed return, but is off for test sources.
+ Under it a ScalaTest suite reports ~1,250-1,450 warnings, and the two largest
+ groups are the idiom itself: an `assert(...)` in trailing position discards an
+ `org.scalatest.Assertion`, and `checkSparkAnswerAndOperator` discards the
+ `(SparkPlan, SparkPlan)` it returns at all but 30 of its ~1,300 call sites.
+ -->
<profile>
- <id>strict-warnings</id>
- <build>
- <plugins>
- <plugin>
- <groupId>net.alchim31.maven</groupId>
- <artifactId>scala-maven-plugin</artifactId>
- <configuration>
- <args>
- <arg>-deprecation</arg>
- <arg>-unchecked</arg>
- <arg>-feature</arg>
- <arg>-Xlint:_</arg>
- <arg>-Ywarn-dead-code</arg>
- <arg>-Ywarn-numeric-widen</arg>
- <arg>-Ywarn-value-discard</arg>
- <arg>-Ywarn-unused:imports,patvars,privates,locals,params,-implicits</arg>
- <arg>-Xfatal-warnings</arg>
- </args>
- </configuration>
- </plugin>
- </plugins>
- </build>
+ <id>strict-warnings</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>net.alchim31.maven</groupId>
+ <artifactId>scala-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>scala-compile-first</id>
+ <configuration>
+ <args>
+ <arg>-deprecation</arg>
+ <arg>-unchecked</arg>
+ <arg>-feature</arg>
+ <arg>-Xlint:_</arg>
+ <arg>-Ywarn-dead-code</arg>
+ <arg>-Ywarn-numeric-widen</arg>
+ <arg>-Ywarn-value-discard</arg>
+ <arg>-Ywarn-unused:imports,patvars,privates,locals,-implicits</arg>
Review Comment:
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.
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -486,11 +486,17 @@ case class CometScanRule(session: SparkSession)
// which bucket's per-bucket S3 settings the single native FileIO consumes (see
// catalogProperties below), so this must run before we build them. The task list is
// handed to `extract` below so the reflective accessor runs once per scan.
+ // The task list is cast to `java.util.List[AnyRef]` rather than left at the
+ // `java.util.List[_]` that `getTasks` returns: a wildcard element type makes this
+ // pair infer a top-level existential, which `-Xlint:existential` rejects. The list
+ // is erased at runtime, so the cast costs nothing.
val (icebergTasks, taskValidation) =
try {
IcebergReflection.getTasks(scanExec.scan) match {
case Some(tasks) =>
- (tasks, CometScanRule.validateIcebergFileScanTasks(tasks, s3CompliantSchemes))
+ (
+ tasks.asInstanceOf[java.util.List[AnyRef]],
Review Comment:
`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.
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometIcebergNativeScan.scala:
##########
@@ -353,7 +353,7 @@ object CometIcebergNativeScan extends CometOperatorSerde[CometBatchScanExec] wit
val equalityIds = equalityIdsMethod
.invoke(deleteFile)
.asInstanceOf[java.util.List[Integer]]
- equalityIds.forEach(id => deleteBuilder.addEqualityIds(id))
+ equalityIds.forEach(id => { val _ = deleteBuilder.addEqualityIds(id) })
Review Comment:
Does `deleteBuilder.addAllEqualityIds(equalityIds)` work here? It avoids the discarded value entirely.
##########
spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala:
##########
@@ -794,7 +794,7 @@ abstract class CometTestBase
.withType(schema)
// TODO we need to shim this and use withRowGroupSize(Long) with later parquet-hadoop versions to remove
// the deprecated warning here
- .withRowGroupSize(rowGroupSize.toInt)
+ .withRowGroupSize(rowGroupSize)
Review Comment:
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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998814690
##########
spark/src/main/scala/org/apache/comet/rules/CometScanRule.scala:
##########
@@ -486,11 +486,17 @@ case class CometScanRule(session: SparkSession)
// which bucket's per-bucket S3 settings the single native FileIO consumes (see
// catalogProperties below), so this must run before we build them. The task list is
// handed to `extract` below so the reflective accessor runs once per scan.
+ // The task list is cast to `java.util.List[AnyRef]` rather than left at the
+ // `java.util.List[_]` that `getTasks` returns: a wildcard element type makes this
+ // pair infer a top-level existential, which `-Xlint:existential` rejects. The list
+ // is erased at runtime, so the cast costs nothing.
val (icebergTasks, taskValidation) =
try {
IcebergReflection.getTasks(scanExec.scan) match {
case Some(tasks) =>
- (tasks, CometScanRule.validateIcebergFileScanTasks(tasks, s3CompliantSchemes))
+ (
+ tasks.asInstanceOf[java.util.List[AnyRef]],
Review Comment:
Done. getTasks and its helpers return Option[java.util.List[AnyRef]], so the cast now lives with the other reflection casts.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#issuecomment-5653975225
> 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: https://github.com/apache/datafusion-comet/issues/5893
please let me know if I need to 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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on code in PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#discussion_r3998842774
##########
pom.xml:
##########
@@ -859,29 +859,85 @@ under the License.
</pluginManagement>
</build>
</profile>
+ <!--
+ Compile Scala with warnings promoted to errors. Not active by default; run it
+ explicitly, e.g. `./mvnw test-compile -Pspark-3.5 -Pstrict-warnings`.
+
+ This passes on the Scala 2.12 profiles. The 2.13 profiles (spark-4.0 and later)
+ still report warnings that 2.12 does not raise at all, dominated by
+ `-Xlint:nonlocal-return` (a `return` inside a closure, which the compiler
+ implements by throwing) and non-exhaustive matches. Clearing those means
+ restructuring control flow rather than annotating it, so they are left for a
+ follow-up rather than silenced here.
+
+ `args` is configured per execution rather than on the plugin, because main and
+ test sources warrant different flags (see `-Ywarn-value-discard` below). An
+ execution's `args` replaces the plugin-level list instead of appending to it, so
+ each list below is self-contained.
+
+ Two lints are deliberately absent from both lists:
+
+ `-Ywarn-unused:params` reports ~90-120 parameters per profile, and essentially
+ all of them are structurally unfixable rather than dead: `Native.scala` is 64
+ `@native` declarations whose parameters have no body to be used in, and the rest
+ are cross-version shims under `src/main/spark-3.x`, `spark-4.x` and friends that
+ take a parameter to satisfy the signature of the Spark version they shim. Nor can
+ those be annotated away one by one: the set differs between Scala 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:_` then reports via
+ `-Xlint:nowarn` and `-Xfatal-warnings` turns into a build failure.
+
+ `-Ywarn-value-discard` stays on for main sources, where a discarded result is
+ usually a dropped builder or a swallowed return, but is off for test sources.
+ Under it a ScalaTest suite reports ~1,250-1,450 warnings, and the two largest
+ groups are the idiom itself: an `assert(...)` in trailing position discards an
+ `org.scalatest.Assertion`, and `checkSparkAnswerAndOperator` discards the
+ `(SparkPlan, SparkPlan)` it returns at all but 30 of its ~1,300 call sites.
+ -->
<profile>
- <id>strict-warnings</id>
- <build>
- <plugins>
- <plugin>
- <groupId>net.alchim31.maven</groupId>
- <artifactId>scala-maven-plugin</artifactId>
- <configuration>
- <args>
- <arg>-deprecation</arg>
- <arg>-unchecked</arg>
- <arg>-feature</arg>
- <arg>-Xlint:_</arg>
- <arg>-Ywarn-dead-code</arg>
- <arg>-Ywarn-numeric-widen</arg>
- <arg>-Ywarn-value-discard</arg>
- <arg>-Ywarn-unused:imports,patvars,privates,locals,params,-implicits</arg>
- <arg>-Xfatal-warnings</arg>
- </args>
- </configuration>
- </plugin>
- </plugins>
- </build>
+ <id>strict-warnings</id>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>net.alchim31.maven</groupId>
+ <artifactId>scala-maven-plugin</artifactId>
+ <executions>
+ <execution>
+ <id>scala-compile-first</id>
+ <configuration>
+ <args>
+ <arg>-deprecation</arg>
+ <arg>-unchecked</arg>
+ <arg>-feature</arg>
+ <arg>-Xlint:_</arg>
+ <arg>-Ywarn-dead-code</arg>
+ <arg>-Ywarn-numeric-widen</arg>
+ <arg>-Ywarn-value-discard</arg>
+ <arg>-Ywarn-unused:imports,patvars,privates,locals,-implicits</arg>
Review Comment:
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?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "athlcode (via GitHub)" <gi...@apache.org>.
athlcode commented on PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#issuecomment-5653949692
> 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
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] chore: Fix Scala code warnings [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #5876:
URL: https://github.com/apache/datafusion-comet/pull/5876#issuecomment-5803766660
Sorry @athlcode there is another conflict. I wonder if it is worth disabling the enforcement in this pr so we can get the bulk of the changes merged then follow up with a smaller enforcement PR?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org