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: [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]
