sunchao commented on PR #5543:
URL:
https://github.com/apache/datafusion-comet/pull/5543#issuecomment-5798538851
Found **three P2 issues** at head `20dfd083`, using five review agents and
independent reproduction.
1. **Collated prefix pruning can discard matching rows.**
[ArrowCachedBatchSerializer.scala:374](https://github.com/apache/datafusion-comet/blob/20dfd08384e0b26bb23f92580d31740c0e337c43/spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/ArrowCachedBatchSerializer.scala#L374)
now admits collated strings into Spark’s prefix-pruning rewrite. That rewrite
assumes matching prefixes preserve character counts, which collations can
violate. With `UTF8_LCASE`, values beginning with `İ` match prefix `i\u0307`,
but caching changed the result from five rows to zero. Disabling pruning
restores them; base Comet also returns them correctly. Spark’s default cache
shares this latent bug. Exclude unsafe collated `StartsWith` predicates while
retaining equality/range pruning.
2. **A tiny raw buffer can retain the entire compressed allocation.**
[CachedBatchIpc.scala:345](https://github.com/apache/datafusion-comet/blob/20dfd08384e0b26bb23f92580d31740c0e337c43/spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/CachedBatchIpc.scala#L345)
makes every selected buffer a slice of one allocation. Arrow’s raw fallback
preserves that shared ownership after other buffers decompress. In a 4,096-row
Boolean/string batch, a **512-byte bitmap pinned an extra 2 MiB**, raising live
Arrow allocation from approximately **2.00 MiB to 4.03 MiB** versus the base
format. It releases on batch close, so this is excess live memory, not a
permanent leak. Isolate raw fallback buffers when mixed with compressed buffers.
3. **Two new value tests compare the cache against itself.**
At
[CometInMemoryCacheSuite.scala:1709](https://github.com/apache/datafusion-comet/blob/20dfd08384e0b26bb23f92580d31740c0e337c43/spark/src/test/scala/org/apache/comet/exec/CometInMemoryCacheSuite.scala#L1709),
also line 1744, Spark substitutes the cached relation into the supposedly
uncached reference query. Changing a stored nested value to `777777` still
passed both tests’ comparisons; a reference collected before caching caught it.
Collect expected rows before cache registration. This demonstrates a test
weakness, not independent production corruption.
The overall design is reasonable: `CachedBatchIpc` separates format handling
from lifecycle management, and per-partition `Projection` avoids repeated
schema traversal. Explicit codec cleanup is justified. Performance remains
workload-dependent: a warmed component comparison showed smaller payloads and
faster narrow reads, but slower writes and full-width reads. Those measurements
are not whole-query results.
Validation passed **59 focused tests**, **147,600 independent value
comparisons**, and **2.25 GB roundtrips with both codecs**, confirming the
previous array-size issue is fixed.
[CI](https://github.com/apache/datafusion-comet/actions/runs/35872317193) has
24 successful and 14 skipped checks; it tested a merge commit. Full upstream
Spark SQL and Iceberg suites remain unverified.
--
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]
