andygrove commented on PR #5543: URL: https://github.com/apache/datafusion-comet/pull/5543#issuecomment-5684627254
Thanks @comphead — the Major is real and the fix is in, along with everything else. Replies are on the individual threads; the two nits from your summary are done too (`Projection` takes an `IndexedSeq[Field]`, and `hydrateDictionaries` is now `decodeDictionaries`). Two things worth pulling up here rather than leaving buried in a thread. The first is that I went wider than dropping `FixedSizeBinaryVector` from `isArrowBacked`. That closes the case you found, but not the class it belongs to: `isArrowBacked` answers for the top-level vector and never looks at children, so a struct whose child is a `LargeVarCharVector` passes it today and is stored with 64-bit offsets and read back with 32-bit. The length check you proposed can't catch that one either, because `LargeUtf8` and `Utf8` are both three buffers, so the totals agree and the values are still wrong. The write path now asks the question directly — do this batch's vectors already carry the Arrow types the reader will rebuild, recursively — and converts the batch when they don't, which is the path it already takes for non-Arrow input. That makes the fast path safe by construction rather than safe for the vectors we happened to enumerate. The second is that the length check does carry its weight. I mutated it away and re-ran the test written for it: it fails with "no exception was thrown", which is exactly the silent-wrong-answer mode rather than a crash. So the read path really was returning an answer from a layout it had never checked. Both checks also make the failure modes distinct in a way that should help later: a writer that produces an unexpected layout is now converted at cache time, and a payload that somehow still disagrees at read time fails with an error naming the mismatch rather than an `ArrayIndexOutOfBoundsException` from inside a buffer copy. Also changed while I was in here: an unrecognized body-compression byte is now rejected rather than read as plain bytes — `fromCompressionType` falls back to `NO_COMPRESSION`, so a corrupt payload was decoding to garbage rather than failing. Verified on the default profile: `CometInMemoryCacheSuite` 44/44, `CometInMemoryCacheKryoSuite` and `UtilsSuite` green, `spotless:check` and `scalastyle:check` clean, and the whole thing test-compiles against Spark 3.5 and 4.0 as well as 4.1. -- 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]
