andygrove commented on PR #5051:
URL:
https://github.com/apache/datafusion-comet/pull/5051#issuecomment-5431888717
@sandugood sorry for the long silence, and thanks for chasing it. Merged
latest `apache/main` and have an answer on both halves of your report.
**The crash.** #5138 merged on Aug 7, the day after you tested, so the build
you hit did not have it. It normalizes nested field nullability, which is
exactly the `expected Struct("col_1": Utf8, "col_2": Boolean, ...)` versus
`found Struct("col_1": Utf8, "col_2": non-null Boolean, ...)` mismatch you saw.
It is in this branch now. If you get a chance to retest, that is the one thing
I still cannot verify locally, since Iceberg MOR is the input I have no
reproduction for.
**The performance regression.** You were right, and it is a real property of
the cache format rather than anything specific to your pipeline. I reproduced
it.
`CometCachedBatch` stores each cached batch as one compressed Arrow IPC
stream covering every cached column. `convertCachedBatchToColumnarBatch`
decodes that whole stream and then projects, so read cost does not fall as the
projection narrows. Spark's `DefaultCachedBatch` stores columns separately and
only decodes the ones the scan selected.
Measured on 5M rows, 6 columns (3 longs, 3 strings), cached and then read
repeatedly by Spark operators, comparing the two serializers directly:
| Read shape | Spark cache | Comet cache | Ratio |
|------------|------------:|------------:|------:|
| 1 of 6 columns | 353 ms | 1175 ms | 3.3x slower |
| 3 of 6 columns | 1078 ms | 1859 ms | 1.7x slower |
| 6 of 6 columns | 1227 ms | 1728 ms | 1.4x slower |
| Materialize cache | 5050 ms | 2468 ms | 2.0x faster |
Writing the cache got faster, reading it got slower, and a
cache-once/read-many pipeline that projects a subset of a wide relation ends up
behind. That matches your under 10 minutes going to over 15. Your later note
that there is no regression without `.cache()` fits too: the cost is entirely
on the cached read path.
Two things I got wrong that are worth stating plainly, since they are why
this was not caught here:
1. **The benchmark in this PR never compared against Spark's cache format.**
`spark.sql.cache.serializer` is a static config, so one session cannot
materialize both formats. Both benchmark cases read a `CometCachedBatch`, and
"Comet cache disabled" meant Spark execution over Comet's payload. The 1.5x I
quoted was not a speedup over Spark's cache. I have corrected the PR
description and relabelled the cases.
2. **`InMemoryRelation` memoizes the resolved serializer in a JVM-static
field.** My first attempt at a cross-serializer measurement read
`DefaultCachedBatch` in every phase and showed no regression at all, vacuously.
It needs `InMemoryRelation.clearSerializer()` between sessions.
**What is in the branch now**, on top of the `apache/main` merge:
* The benchmark relation is widened with string columns and has narrow (1 of
6) and full (6 of 6) projection cases, so the effect is visible rather than
invisible.
* The missing baseline is stated at the materialization site, so nobody
reads those numbers as a Spark comparison again.
* The read cost is documented on `spark.comet.exec.inMemoryCache.enabled`.
Fixing it properly means making decode proportional to the projection, most
likely one Arrow stream per column so only the selected ones are inflated. That
is a change to the cache format itself and too large to fold in here, so it is
filed as #5484 with the numbers and the options.
The feature stays off by default, which I think is the right call given the
above. @peterxcli @comphead this changes what the benchmark numbers in the
description mean, so worth a look if you have already reviewed them.
--
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]
