andygrove opened a new pull request, #5170:
URL: https://github.com/apache/datafusion-comet/pull/5170
## Which issue does this PR close?
No issue filed — mechanical compiler-warning cleanup, split out of #5141.
## Rationale for this change
Scala 2.13 deprecates the implicit `Array` → `immutable.IndexedSeq`
conversion because it silently copies. Comet triggers it at 8 sites, plus one
related *"passing an explicit array value to a Scala varargs method"*
deprecation, for **9 of the 127 warnings** under the 2.13 profiles.
The compiler suggests `.toIndexedSeq`. That is a no-op change: the
deprecated implicit is defined as
```scala
implicit def copyArrayToImmutableIndexedSeq[T](xs: Array[T]): IndexedSeq[T] =
if (xs eq null) null
else new ArrayOps(xs).toIndexedSeq
```
so `.toIndexedSeq` performs exactly the copy that is already happening — it
just makes it visible. (The only behavioral difference is `null`: the implicit
yields a `null` Seq, the explicit call NPEs. None of these nine arrays can be
`null`.) So the copies are all *correct* today; the question is whether they
are *needed*.
At 7 of the 9 sites they are not — an `Array` is materialized and then
immediately has to become a `Seq` again, so this PR removes the round trip
rather than annotating it.
## What changes are included in this PR?
**Round trips removed (7 sites):**
- `CometLocalTableScanExec.unsafeRows` is built as an `IndexedSeq` instead
of an `Array`, since its only consumers are `.length` and
`SparkContext.parallelize`, which needs a `Seq`. Drops one copy of the
projected rows.
- `CometScanExec.setFilesNumAndSizeMetric` is private to Comet and only sums
file counts/sizes, so it now takes an `Array[PartitionDirectory]` and
`selectedPartitions` converts the `listFiles` result once (the `.toArray` moves
from the end of the block onto the `listFiles` call). Same number of copies as
before, two fewer conversions.
- `Utils.stringToSeq` splits, trims and filters through an iterator, so the
two intermediate arrays are gone.
- `CometTestBase.internalCheckSparkAnswer` holds the collected rows as a
`Seq[Row]`, which is what both `checkAnswerWithTolerance` and
`checkCometAnswer` take.
- `Tables.df` hands its per-row values array directly to `GenericRow` —
which is what `Row.fromSeq` wraps it in anyway — removing two array copies per
generated row on the TPC-H/TPC-DS data generation path. It also maps over the
`StructType` (itself a `Seq[StructField]`) rather than over `schema.fields`,
which is what removes the varargs warning.
**Copies kept (2 sites):** `FilePartition.maxSplitBytes` and
`FilePartition.getFilePartitions` are Spark APIs that take a `Seq` and only
read from it. `createFilePartitionsForNonBucketedScan` converts the (small)
partition array once with `.toSeq` and uses that for both — which also means
`splitFiles`, the much larger collection, is built as a `Seq` and never copied.
`.toSeq` rather than `.toIndexedSeq` because on 2.12 `Array.toSeq` wraps
instead of copying, and neither form is deprecated.
### Alternatives considered
- **`ArraySeq.unsafeWrapArray`**, which the deprecation message recommends:
`scala.collection.immutable.ArraySeq` does not exist in 2.12, so this would not
cross-build.
- **Spark's `.toImmutableArraySeq`**
(`org.apache.spark.util.ArrayImplicits`), which is how Spark itself fixed
these: only present in Spark 4.x — I checked the 3.4.3 and 3.5.8 jars and the
class is absent — so it would need a shim to save a planning-time copy.
- **Retyping `CometScanExec.selectedPartitions` to
`Seq[PartitionDirectory]`**, which would eliminate all three conversions in
that file: rejected because the bucketed path feeds `FilePartition(bucketId,
files: Array[PartitionedFile])`, so `groupBy` over a `Seq` would trade one
array copy for one copy per bucket.
## How are these changes tested?
No new tests — no intended behavior change.
- Warnings under the default profile (Spark 4.1 / Scala 2.13): **127 →
118**, with all 9 of these gone and none added. Verified by diffing the full
sorted warning list before and after.
- Scala 2.12 (`-Pspark-3.5`): **15 → 15** — these warnings do not exist on
2.12, and none were introduced.
- `test-compile` passes on all five profiles: default, `-Pspark-3.4`,
`-Pspark-3.5`, `-Pspark-3.5 -Pscala-2.13`, `-Pspark-4.0`.
- `spotless:check` and `scalastyle:check` pass.
- 207 tests pass across `CometExecSuite` (covers the bucketed scan, dynamic
partition pruning and local table scan paths), `ParquetReadV1Suite` (V1 scan
planning) and `CometConfSuite` (`stringToSeq` via the seq-valued configs).
Every one of them also exercises the changed `CometTestBase` helper.
- `Tables.scala` is only reachable with `dbgen`/`dsdgen` present, so it is
not covered by CI; it is compile-checked and the `GenericRow` substitution is
what `Row.fromSeq` expands to.
--
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]
