perf: project cached batches by buffer selection, prune on collated strings - #5543
perf: project cached batches by buffer selection, prune on collated strings#5543andygrove wants to merge 7 commits into
Conversation
…trings Follow-up to apache#5051, applying items from apache#5487. Replace the per-column Arrow IPC stream layout of `CometCachedBatch` with a single encapsulated IPC record batch message per cached batch, carrying no Schema message and no end-of-stream marker. The reader rebuilds the schema from the cached relation's attributes, so a wide relation no longer repeats the same schema bytes once per cached batch. Compression moves from a whole-payload Spark codec to Arrow's per-buffer IPC compression. That is what makes projection cheap: the message metadata records every buffer's offset and length in the body, so `CachedBatchIpc.readProjected` copies out only the byte ranges of the columns a scan selected and decompresses just those. This subsumes the separate "drop the schema message" item, since there is no longer a per-column stream to frame. Dictionary-encoded columns are decoded before being stored: a payload with no schema message cannot describe a dictionary encoding. The codec defaults to zstd, and lz4 is deliberately not offered. Arrow's lz4 is commons-compress's pure-Java implementation, unrelated to the JNI-accelerated lz4-java behind `spark.io.compression.codec`. Over a 200k-row six-column relation it measured 205s to write against 347ms for zstd, while also producing larger output, so no workload prefers it. zstd also beats storing batches uncompressed on both axes (347ms and 2 MiB against 1743ms and 13 MiB), because the bytes it saves cost more to copy and store than compressing them costs. Decompression is done here rather than left to `VectorLoader`, which leaks: `VectorLoader.loadBuffers` collects a field's decompressed buffers into a local list and releases them only after the whole field loads, so a buffer that fails to decompress strands every buffer of that field decompressed before it. A string column reaches this, its offsets buffer decompressing before its data buffer throws. Also track statistics bounds for collated string columns, comparing with the collation's own ordering through a new `CometTypeShim.compareStrings`. Matching the bare `StringType` object excluded collated columns, which then got null bounds and no pruning. Benchmark over a 5M-row six-column relation, keeping the cached scan native against falling back to a Spark cache scan and converting: 1.3x on a repeated scan, 1.3x on a narrow projection and 2.3x on a full projection.
…on layout Cleanup pass over the cache format change. No behaviour change. Drop the `compareStrings` shim in favour of `TypeUtils.getInterpretedOrdering`. That method is public with the same signature on every supported Spark version, and on Spark 4 it resolves a `StringType` through `CollationFactory.fetchCollation(collationId).comparator` -- the comparison the shim was reaching for. So the collation awareness comes from Spark itself and the shim, its Spark 3.x stub and the hand-rolled per-type `compare` all go. The ordering is now resolved once per column per partition rather than being re-dispatched on the `DataType` twice per row. Build the projection's index layout once per partition instead of per batch. The node, buffer and variadic index arithmetic is a pure function of the cached schema and the selected columns, but it walks every field of the relation, so recomputing it per batch made the bookkeeping O(total columns) against O(selected columns) of useful work -- worst in the wide-relation, narrow-projection case the format exists for. `CachedBatchIpc.Projection` now holds that layout and the projected schema, and owns the whole decode; `ProjectedBatch` is left with ownership only. This also puts the projected schema next to the code that packs buffers in the same order, an invariant that previously spanned two files unstated. Smaller cleanups: use Arrow's `DataSizeRoundingUtil.roundUpTo8Multiple` rather than open-coding IPC body alignment; size the serialization buffer from the record batch's known body length instead of growing from 32 bytes; resolve decompressors once instead of per batch; share the dictionary lookup guard between `Utils.combineDictionaryProviders` and the cache writer; read the codec config through one helper carrying the driver-vs-executor rationale; and collapse the duplicated compressed-buffer predicate and scramble loop in the test helper. Corrects two `Utils` scaladocs that still described the per-column stream format this change replaced. Benchmark and codec figures in the docs re-measured against the current code.
arrow-compression ships META-INF/services/org.apache.arrow.vector.compression.CompressionCodec$Factory. The shade plugin copies it verbatim without a ServicesResourceTransformer, so the jar declared a provider for Spark's own unshaded Arrow interface while naming a class that exists here only under the relocated package. Every ServiceLoader lookup Spark's Arrow made then failed with a ServiceConfigurationError, which took CompressionCodec.Factory's static initializer down with it and broke unrelated Arrow IPC reads, including mapInArrow. Add ServicesResourceTransformer so the service file name and its contents are both relocated. arrow-compression is the only bundled artifact that ships one. Also drop an unused NonFatal import that scalafix flagged.
"releases its vectors when a column fails part way through" zeroed the last 16 bytes of a compressed buffer and required the read to fail. Whether that fails is a property of the zstd runtime, not of Comet: the cached payload is byte-identical across Spark versions, but Comet takes zstd-jni from Spark rather than from arrow-compression, and 1.5.5 (Spark 3.4, 3.5) decodes that frame while 1.5.7 (Spark 4.x) reports it corrupt. So the test passed on 4.x and failed on 3.4 and 3.5. The scenario it claimed to cover is also unreachable: CachedBatchIpc decompresses every selected buffer before VectorLoader runs, so no content corruption can fail part way through the load. The two remaining leak tests corrupt a frame from its header onwards, which every zstd release rejects, and already cover a failure at a column's first buffer and a failure after an earlier buffer of the same column decoded. Records the constraint on scramble so a future test does not reach for a tail-only corruption again, and drops the now unused truncateColumn helper and the dictionary fixture's payload argument.
comphead
left a comment
There was a problem hiding this comment.
Thanks @andygrove should we also test/benchmark nested data?
…enchmark Addresses review feedback asking whether nested data should be tested and benchmarked. Nested columns were already round-tripped, but only under a full projection, which cannot see the part of the format that is nontrivial for them. A flat column always owns one field node and two or three buffers; a nested one owns a run as long as its subtree, and selecting every column covers the whole sequence however it is partitioned. So the buffer-span arithmetic was only exercised in the one shape where getting it wrong does not show. Adds two tests over a six-column relation whose middle four columns are a struct, an array, a map and a struct wrapping an array: - Each column takes its turn as the sole projection while the other five are corrupted, so a run computed short or long is caught by reaching into a corrupted neighbour. - Values are compared against the uncached query across single-column, paired and out-of-order projections. Row counts cannot catch a window that is misaligned but still decompresses, and out-of-order is the case a full projection cannot stand in for. The per-column statistics test now runs over the nested relation too, since a nested column's recorded size is the sum of its whole subtree. Both new tests fail if fieldNodeCount stops recursing into children. In the benchmark, adds the three projection widths over a relation of struct columns, and asserts the width each case claims. That assertion caught the existing "full projection (6 of 6 columns)" case reading three: count() over a non-nullable column is rewritten to count(1) by NullPropagation, which prunes the column out of the scan, and only k, s1 and s2 were nullable -- and those only incidentally, because Remainder can divide by zero. Every column of both relations is now nullable so count(c) genuinely reads c, and the documented numbers are regenerated. Array and map columns are left out of the benchmark deliberately: the baseline arm needs Spark's cache scan to bridge into Comet operators, and CometSparkToColumnarExec declines ArrayType and MapType, so for those the arm does not exist and the two cases stop measuring the same boundary. The docs say so rather than leaving it to be rediscovered.
|
Good call — this turned out to be worth doing, because the nested coverage that existed was in the one shape that can't fail. Nested columns were already round-tripped, but only under There are now two tests over a six-column relation whose middle four are a struct, an array, a map and a struct wrapping an array. The first gives each column a turn as the sole projection with the other five corrupted, so a run computed short or long by a buffer gets caught reaching into a corrupted neighbour. The second compares values against the uncached query across single-column, paired and out-of-order projections — row counts come from the record batch header, so they can't catch a window that is misaligned but still decompresses, and an out-of-order projection is the case a full one genuinely cannot stand in for. The per-column statistics test now runs over the nested relation too, since a nested column's recorded size is the sum of its subtree. Both new tests fail if The format itself needed no changes, so this is coverage rather than a fix. On the benchmark, I added the three projection widths over a relation of struct columns:
The gap is wider than the flat relation's at every width, which is what you'd expect — the conversion the left column pays scales with values per row, not with columns. I left arrays and maps out of the benchmark on purpose, and said so in the docs. The left column needs Spark's cache scan to bridge into Comet operators, and One thing your question shook out that I should flag: while adding the nested cases I made |
…ection-projection
Which issue does this PR close?
Part of #5487. Ticks these boxes:
Not addressed here: typed readers for the row path (#5485 has no established cause yet, so this would be optimising ahead of a diagnosis) and background prefetch.
Rationale for this change
#5051 stores each cached column as its own compressed Arrow IPC stream, so a scan decodes only the columns it projects. That works, but it pays an Arrow schema block and compression framing per column per batch, and gives up cross-column compression: footprint grows 2.5% at 6 columns and 32% at 60.
Spark's
ArrowCachedBatchSerializer(SPARK-57268) reaches the same projection-proportional decode with no per-column framing at all. It keeps one RecordBatch per cached batch and parses the IPC message flatbuffer, which lists every buffer's offset and length within the body, to copy out only the byte ranges belonging to the selected columns. It depends on Arrow's native per-buffer compression rather than wrapping the whole payload in a SparkCompressionCodec.That approach dominates the current design on footprint while keeping the projection win, so this PR adopts it.
Separately,
tracksBoundsmatchescase StringType, which a collatedStringTypedoes not equal. Collated columns therefore get null bounds andbuildFilterdeclines to push predicates on them. That is correct but loses pruning that Spark manages.What changes are included in this PR?
Cached batch format.
CometCachedBatch.columns: Array[ChunkedByteBuffer]becomesbytes: Array[Byte]: one encapsulated Arrow IPC record batch message and its body, with no Schema message and no end-of-stream marker. The reader rebuilds the schema from the cached relation's attributes viaUtils.toArrowSchema, so a wide relation no longer repeats the same schema bytes once per cached batch. NewCachedBatchIpcowns the format — the field-node, buffer-span and variadic-count arithmetic, and the projected read that copies only the selected columns' buffers into a single off-heap allocation.Compression moves from a whole-payload Spark codec to Arrow's per-buffer IPC compression, which is what lets a projected read decompress only what it selected. New
spark.comet.exec.inMemoryCache.compression.codec(zstddefault,noneavailable) and...compression.zstd.level.Arrow's lz4 is deliberately not offered. It is commons-compress's pure-Java implementation, unrelated to the JNI-accelerated lz4-java behind
spark.io.compression.codec. Over a 200k-row six-column relation:zstdnonelz4lz4 is dominated on both speed and size, so nothing prefers it. zstd also beats storing batches uncompressed on both axes, because the bytes it saves cost more to copy and store than compressing them costs. Reads still accept any codec a batch records.
Dictionary-encoded columns are decoded before being stored. A payload with no schema message has nowhere to record either the index type or the dictionary. Comet's native scans do produce such columns, so this is a real path rather than a defensive one.
Decompression is done in
CachedBatchIpcrather than left toVectorLoader. arrow-java 18.3.0 leaks on the failure path:VectorLoader.loadBufferscollects a field's decompressed buffers into a local list and releases them only after the whole field has loaded, so a buffer that fails to decompress strands every buffer of that field decompressed before it. A string column reaches this — its offsets buffer decompresses, then its data buffer throws — so a single corrupt cached batch leaks off-heap for the life of the executor.Collated string pruning.
tracksBoundswidens to anyStringType, and bounds are compared withTypeUtils.getInterpretedOrdering(dataType)— Spark's own interpreted ordering for the type, which on Spark 4 resolves aStringTypethroughCollationFactory.fetchCollation(collationId).comparator. So bounds are recorded with the same ordering the partition filter Spark generates over that column uses, the collation awareness comes from Spark at every supported version with no shim, and the ordering is resolved once per column instead of re-dispatching on theDataTypeper row. This also replaces the hand-rolled per-typecompare.Reading builds one
CachedBatchIpc.Projectionper partition, holding the projected schema and the node/buffer/variadic index layout. That arithmetic walks every field of the cached relation, so computing it per batch would make the bookkeeping O(total columns) against O(selected columns) of useful work — worst in exactly the wide-relation, narrow-projection case this format exists for.Dependency. Adds
org.apache.arrow:arrow-compression. Already covered by the existingorg.apache.arrow:*shade include, so it relocates with the rest of Arrow; itscommons-compressandzstd-jniare excluded and come from Spark, which ships both on every supported version.How are these changes tested?
CometInMemoryCacheSuitekeeps its existing coverage, with the format-dependent tests rewritten against the new layout:nonetakes a different path on read and was broken until this test was written.BinaryType.VectorLoaderbehaviour above. Both assert the decode error surfaces as itself rather than as an Arrow reference-count error, which is what catches a cleanup path releasing the shared body twice.CometCachedBatchHelperre-derives the IPC buffer arithmetic independently rather than calling intoCachedBatchIpc, so the assertions built on it cannot pass by inheriting a bug from the code under test.Nested columns get their own projection coverage, over a six-column relation whose middle four are a struct, an array, a map and a struct wrapping an array. They were previously round-tripped only under
SELECT *, which cannot exercise the span arithmetic: a flat column always owns one field node and two or three buffers, a nested one owns a run as long as its subtree, and a full projection covers the whole buffer sequence however it is partitioned. So one test gives each column a turn as the sole projection with the other five corrupted, and another compares values against the uncached query across single-column, paired and out-of-order projections — a row count comes from the record batch header, so it cannot catch a window that is misaligned but still decompresses. The per-column statistics test runs over the nested relation too. Both new tests fail iffieldNodeCountstops recursing into children.Also run:
CometInMemoryCacheKryoSuite,CometExecSuite,UtilsSuite. Compiles clean against Spark 3.5, 4.0 and 4.1. The shaded jar was checked to confirm arrow-compression relocates and that commons-compress and zstd-jni are not bundled.CometInMemoryCacheBenchmark(Apple M3 Max, JDK 17, Spark 4.1, release build), over a 5M-row relation of six flat columns:CometInMemoryTableScanAnd over a 1M-row relation of six columns whose middle three are structs, one nested two levels deep:
CometInMemoryTableScanverifyPlannow asserts the projection width each case claims, which is what corrected the flat table above: the previousfull projection (6 of 6 columns)row was reading three columns, becausecount()over a non-nullable column is rewritten tocount(1)byNullPropagationand the column is then pruned out of the scan. Onlyk,s1ands2were nullable, and only incidentally, becauseRemaindercan divide by zero. Every column of both relations is nullable now socount(c)genuinely readsc. A real six-column read is 1.9x, not the 2.2x previously published for a three-column one.Arrays and maps are deliberately not in the benchmark. The baseline arm needs Spark's cache scan to bridge into Comet operators, and
CometSparkToColumnarExecdeclinesArrayTypeandMapType, so for a query projecting one of those that arm does not exist and the two cases stop measuring the same boundary. They are covered in the suite instead.As with #5051, both columns read the same Comet-written
CometCachedBatchand Comet execution is on in both, so this measures keeping the cached scan native against falling back to a Spark cache scan and converting — not Comet against Spark execution, and not a comparison with Spark's own cache format.Notes for reviewers
spark.comet.exec.inMemoryCache.enabledstaysfalse, as it has been since feat: add experimental native support for in-memory cache, disabled by default #5051, so none of this reaches a default configuration. Everything below the config, including the change of cached format, only affects users who have opted in to the experimental native cache. The config is static: its value at startup is what decides whether Comet's cache serializer is installed at all.spark/benchmarksis in.gitignore, so Comet does not currently commit results files the way Spark does. Doing it properly needs that directory un-ignored plus a workflow to regenerate them, or the numbers rot — worth its own decision rather than being slipped in here. The measured tables live in the new docs page instead.