andygrove opened a new pull request, #5179: URL: https://github.com/apache/datafusion-comet/pull/5179
## Which issue does this PR close? Part of #5178. ## Rationale for this change The contributor guide explains how Comet's subsystems work but does not record the invariants that must hold when changing them. `native_shuffle.md` and `ffi.md` are architectural explainers, there was no page covering native memory management at all, and nothing described when native code should defer to arrow-rs or DataFusion instead of hand-rolling logic. That gap costs twice. Reviewers re-derive the same invariants on every PR, and contributors who have not read the relevant subsystem produce code that looks correct while violating rules nobody wrote down. The audit behind #5104 filed sixteen follow-ups, two of them real bugs, and none of that knowledge lived anywhere a contributor would look. Each page follows the shape of `optimizing_expressions.md`: enough narrative to orient someone new, then a rules section carrying the checkable content. ## What changes are included in this PR? Three contributor guide changes. **`working_with_arrow.md` (new)** covers deciding whether to write native array code at all, then writing it correctly. The reuse half gives the order to check arrow-rs, DataFusion, and `datafusion-spark` before hand-rolling, and reproduces the list of places Comet diverges from arrow deliberately so those are not re-raised in review. The correctness half covers the Arrow API traps that produced real bugs, including that `unary` and `binary` run the closure on null slots while `try_unary` and `try_binary` skip them, which is what separates #5093 from the code that gets it right. **`memory_management.md` (new)** covers the nine native pool types along the two axes that actually distinguish them (greedy versus fair spill, and scope), the hard split where off-heap mode accepts only the two unified pools and on-heap accepts the other seven, how native accounting reaches Spark through `CometTaskMemoryManager`, and where spilling is actually implemented. It cross-references `ffi.md` rather than restating buffer ownership: that page covers who owns a buffer, this one covers who is charged for it. **`native_shuffle.md` (extended)** gains a rules section covering the invariants that keep shuffle output readable by Spark, and its existing memory section now links to the new memory management page instead of explaining pool mechanics inline. Every rule in all three pages cites the source file it was verified against, a filed issue, or both. Rules that could not be grounded in real code were dropped rather than softened into general advice. Writing the shuffle rules surfaced three pre-existing errors on that page, each of which would have contradicted the new section, so they are corrected here: the Rust-side table pointed at `native/core/src/execution/shuffle/`, which is now a re-export of `native/shuffle/`; the partitioning description said `hash % num_partitions` where the code uses `pmod`; and the memory section named a `PartitionBuffer` type that no longer exists. The section also records a scoping correction. The radix sort that #5104 lists under shuffle, and the CRC32, CRC32C, and Adler32 checksums, are both reachable only from `CometShuffleExternalSorter`, so they belong to JVM shuffle rather than the native path. The native writer currently emits no checksums at all. Finally, `native_shuffle.md` picks up six blank-line deletions from `prettier --write`. The file already failed `prettier --check` on `main` because of prettier version drift, so editing it meant absorbing the reformat. ## How are these changes tested? Documentation only, no code changes. The docs build was verified locally with Sphinx. The build succeeds, the new pages resolve in the toctree, and the cross-references between the three pages resolve with no new warnings relative to `main` (the repo carries 55 pre-existing warnings from generated user-guide pages that a source-only build skips). `prettier --check` passes on the contributor guide. The factual claims were verified by reading the source each rule cites, including tracing control flow to confirm that a cited mechanism is actually reachable and actually produces the stated effect. -- 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]
