[acruise]

Which issue does this PR close?

Part of #6765. The issue asks for the views to be detached once per row; this does it once per
batch instead, for the reason below, and leaves the issue open for the maintainers to decide
whether that bound is acceptable.

Rationale for this change

ArrowWriter's row path reads each array, map and struct of an UnsafeRow through Spark's
getters, which allocate a view of the value for every value: an UnsafeArrayData, an
UnsafeMapData with its two arrays, or an UnsafeRow. An earlier revision of #6730 pointed one
reused view per writer at each value instead, and was 2 to 4 ns per row faster on nested types.
It was dropped in review because a reused view keeps the last row it read reachable, along with
the row's memory, until the writer is dropped.

#6765 proposed keeping the reuse and detaching the views once per row. I implemented that first
and measured it: a pointTo per nested writer per row costs 1 to 3 ns, which is what the reuse
saves, so against main the net was nil on JDK 21 and within noise on JDK 17. The full table is
in #6765 (comment).

This PR detaches once per batch instead, in finish and reset. The cost is then nothing per
row, and the retention is bounded: between rows a writer holds at most one row per nested writer,
and every production caller (RowArrowReader, SparkColumnarArrowReader, both paths in
CometArrowConverters) builds the writer as a local inside its per-batch method and finishes it
there, so a batch that throws drops its views with its frame. Whether one row per nested writer
for the life of one batch is an acceptable bound is the question for review; it is the same
retention #6730's review rejected, now with the alternative measured.

What changes are included in this PR?

spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowWriters.scala:

• ArrayWriter, StructWriter and MapWriter each keep one UnsafeArrayData, UnsafeRow or
  UnsafeMapData and point it at every unsafe value they write, in writeUnsafeRowField and
  writeArrayElements, as the earlier revision of perf: convert Spark's unsafe rows to Arrow straight from their memory #6730 did. The generic path through setValue
  is unchanged.
• A new UnsafeViews holds the views of all the writers under an ArrowWriter, collected once at
  construction through ArrowFieldWriter.collectViews, by type so that each detach is a direct
  call. detach points each at an empty layout: a long[]{0} for an array, long[]{8, 0, 0} for
  a map (a null base would not do for either, pointTo reads a header), and (null, 0, 0) for a
  row.
• ArrowWriter.finish and ArrowWriter.reset call detach. write does not. A flat schema
  collects no views and is untouched.

pom.xml: the strict-warnings profile gets a -Wconf filter for the unused parameter of the
default ArrowFieldWriter.collectViews hook, next to the filters for the other extension hooks that
leave a parameter for overrides. Without it the strict job reports that parameter, and then the
@nowarn annotations on the shuffle managers' getReader/getWriter as unused, because the
compile stops before the phase whose warning they suppress.

spark/src/test/scala/org/apache/spark/sql/comet/execution/arrow/CometArrowWriterSuite.scala:

• the row path keeps no reference to an unsafe row once it is written becomes ... once its batch is finished. It writes 20 rows of nested arrays, maps and structs, each as an unsafe row and as
  a generic row holding the unsafe values (which reach the same views through setValue), with
  and without nulls, and checks that at most five rows (one per nested writer) are reachable from
  the writer before finish or reset and none after. The test fails if the detach is removed.

How are these changes tested?

All 27 tests of CometArrowWriterSuite pass, including the existing checks that the unsafe-row
path matches the generic row path for every type, with collections past 64 elements and strings
of every length.

Throwaway harness, not included: one column of 8192 unsafe rows per shape, every tenth row and
element null, collections of i % 6 elements, all ten shapes warmed together in one JVM, best of
30 rounds after 10 warmup rounds, median of three JVMs. Apple M3 Max, assertions off. ns per row,
main -> this PR:

shape	JDK 21
int	5.8 -> 5.4
string	12.2 -> 11.0
array	28.5 -> 26.0
map<string,string>	50.3 -> 45.6
array<struct<int,string>>	55.4 -> 51.5
array<array>	44.0 -> 42.3
map<int,long>	29.5 -> 26.2
struct<int,string>	21.9 -> 19.5
array<map<string,int>>	97.8 -> 88.4 (see below)
row of 8 (int, long, string, double, array, map, struct, binary)	119.6 -> 113.4

Nested shapes are 3 to 11% faster; the flat columns move within the run-to-run spread of about 1
ns. On JDK 17 the reuse without a per-row detach, which is what this PR measures as, was 2 to 4 ns
per row under main on the same shapes (table in the issue comment linked above).

The array<map<string,int>> figure is not a result of this PR and should not be read as one. That
shape is bimodal on main as well, 92 to 95 ns in some JVMs and 104 to 111 in others, which is
#6757. With -XX:+PrintInlining on five JVMs of the harness, the two modes differ in exactly one
inlining decision: in the final C2 compile of MapWriter.writeArrayElements, the loop over an
array's maps, the call to writeUnsafe (and through it writeEntries) is inline (hot) in every
slow JVM and already compiled into a big method in every fast one. The inline trees below that
point are identical. The decision depends on whether writeUnsafe's own C2 nmethod, which exceeds
InlineSmallCode, has been installed by the time the loop's compile reaches the call site; both
are hot at once on concurrent compiler threads, so it is a race, which is why #6757 saw it only
with many shapes warmed together. This PR neither causes nor fixes it; the same two methods race
on main.

🤖 Generated with Claude Code