Skip to content

chore: remove Array -> Seq round trips flagged by the 2.13 copy deprecation - #5170

Merged
andygrove merged 1 commit into
apache:mainfrom
andygrove:chore/array-to-seq
Jul 31, 2026
Merged

chore: remove Array -> Seq round trips flagged by the 2.13 copy deprecation#5170
andygrove merged 1 commit into
apache:mainfrom
andygrove:chore/array-to-seq

Conversation

@andygrove

Copy link
Copy Markdown
Member

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 Arrayimmutable.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

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.

…cation

Scala 2.13 deprecates the implicit Array -> immutable.IndexedSeq
conversion because it copies. Comet triggers it at 8 sites, plus one
related "explicit array to varargs" deprecation, for 9 of the 127
warnings under the 2.13 profiles.

The implicit is defined as `xs.toIndexedSeq` with a null guard, so
writing `.toIndexedSeq` at each site would only make the existing copy
explicit. At 7 of the 9 sites the Array itself is unnecessary and the
round trip is removed instead:

- CometLocalTableScanExec.unsafeRows is built as an IndexedSeq, so it
  no longer materializes an Array only for SparkContext.parallelize to
  need a Seq back.
- CometScanExec.setFilesNumAndSizeMetric is private to Comet and only
  reads, so it takes an Array and selectedPartitions converts the
  listFiles result once.
- Utils.stringToSeq splits, trims and filters through an iterator.
- CometTestBase.internalCheckSparkAnswer holds the collected rows as a
  Seq, which is what both checkAnswer helpers take.
- Tables passes its per-row values array straight to GenericRow (what
  Row.fromSeq wraps it in) and maps over the StructType rather than its
  fields array.

The two remaining sites are Spark APIs that take a Seq:
FilePartition.maxSplitBytes and FilePartition.getFilePartitions. The
small partition array is converted once with `.toSeq`, which also keeps
splitFiles as a Seq so the larger collection is never copied.
`.toSeq` rather than `.toIndexedSeq` because on 2.12 it wraps.

selectedPartitions stays an Array: the bucketed path feeds
FilePartition(bucketId, files: Array[PartitionedFile]), so a Seq there
would trade one copy for one per bucket.

@mbutrovich mbutrovich left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @andygrove!

@andygrove
andygrove merged commit 2707f21 into apache:main Jul 31, 2026
71 checks passed
andygrove added a commit to andygrove/datafusion-comet that referenced this pull request Jul 31, 2026
The .toIterable protobuf builder fixes and the auto-application-to-()
fixes were split out into apache#5173 and apache#5175. Remove them here so this PR
is limited to the one-off warnings that have no category PR of their
own. apache#5168 and apache#5170 already landed on main.

[skip ci]
andygrove added a commit to andygrove/datafusion-comet that referenced this pull request Jul 31, 2026
The .toIterable protobuf builder fixes and the auto-application-to-()
fixes were split out into apache#5173 and apache#5175. Remove them here so this PR
is limited to the one-off warnings that have no category PR of their
own. apache#5168 and apache#5170 already landed on main.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants