andygrove commented on PR #2204: URL: https://github.com/apache/datafusion-ballista/pull/2204#issuecomment-5226641494
Here's the rest of my read, separate from the CI issues above. **On the format switch and the personas.** This trips a red flag in [user-personas.md](https://github.com/apache/datafusion-ballista/blob/main/docs/source/contributors-guide/user-personas.md) under Persona 3, specifically "wire-format or serialization changes that break externally-produced plans with no compatibility path". The on disk shuffle format moves from Arrow IPC stream to Arrow IPC file with no version marker and no fallback for reading what a previous version wrote. That doesn't make it forbidden, it just means it needs a deliberate call rather than landing quietly, and an entry in the upgrade guide. I appreciate that the description already says this isn't purely additive, I'd only push back on the framing of the cost as roughly two extra range GETs. The bigger cost is that the switch has to land atomically across every writer and reader of that file, and as the CI failures show there are more of those than it first looks. Personas 1 and 2 look fine to me. No stage or task model changes, no AQE changes, and results are unaffected once the format is consistent everywhere. **Crash semantics, one for the "Regressions (honest)" list.** I liked that you wrote that section, so here's one more for it. A stream format file is readable up to the last complete message, and a file format file with no footer doesn't open at all. So a task killed partway through a write now leaves behind a file that hard fails on open rather than one that partially reads. Task level retry probably makes that a non issue in practice, but it's a real behavior change and I'd rather see it named than discovered later. **`write_stream_to_disk` is public.** Its signature is unchanged while its output format changes, so anyone using `ballista-core` as a library gets a silent behavior change with nothing at the type level to warn them. Worth an upgrade guide entry alongside the format switch itself. **The value index code itself.** No concerns. `ValueIndexExec` and `ValueIndexReader` are cleanly additive, nothing constructs the exec yet, and the format agnostic split between "index stores sampled rows and ORDER BY values" and "a per format resolver turns a row into a byte range" reads well to me. The Vortex and Parquet cases in the description make a convincing argument that the split lands in the right place. On the bundled `RangeShuffleWriter` / `RangeShuffleReader` question you raise at the top: I lean toward the tap based shape you have, and I'd rather fix the `leaf_idx == batch_idx` alignment risk directly than give it up. A debug assert comparing the recorded row counts against what the writer actually emitted would turn that silent wrong read into a loud failure, and it keeps the compositional property of one format agnostic `ValueIndexExec` per data writer. Happy to be argued out of it though, since you've been closer to this than I have. -- 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]
