andygrove opened a new pull request, #6227: URL: https://github.com/apache/datafusion-comet/pull/6227
Backport of #5907 to `branch-1.0`. Cherry-picked from `2d1aab3a6abc3461c2320e8a18d203711b1957cf` without conflicts. The diff matches upstream except for line offsets. ## Which issue does this PR close? None. The original doesn't close an issue either: #5907 is part of #5905 (finding J2). Listed in #6201. ## Rationale for this change The bug ships in 1.0.0. `CometShuffleExternalSorter.growPointerArrayIfNecessary` on `branch-1.0` is the same code that `main` had before #5907. It sizes the replacement pointer array from `SpillSorter.getMemoryUsage()`, which counts every allocated data page as well as the array. Spark's `ShuffleExternalSorter` sizes it from the array alone. So the first growth, after `initialSize / 2` records, asks for twice the size of the pages plus the array. Every later growth also counts the pages allocated since. In the new test, which uses a 4 MiB page and a 1,024-entry initial array, the first growth on `branch-1.0` allocates an 8,404,992-byte pointer array. Spark's sizing gives 16,384 bytes. That memory comes out of the same budget as the data pages. The growth can then leave less room for pages, or fail the allocation and force a spill after very few rows. ## What changes are included in this PR? The fix is the original one; see #5907 for the details. `SpillSorter` gains `getPointerArrayMemoryUsage()`, which returns only the in-memory sorter's array size. `growPointerArrayIfNecessary` now uses it. `getMemoryUsage()` is unchanged, so peak memory reporting and spill sizing still include the data pages. No adaptations were needed. ## How are these changes tested? The new test in `SpillSorterSuite`, run locally on `branch-1.0` with the default Spark 4.1 profile and JDK 17: - All 10 tests in `SpillSorterSuite` pass, including the new one. - The bug is present on `branch-1.0`, and the test catches it. With the two production files reverted and the test kept, it fails with `12599296 did not equal 4210688`. That is the page plus an 8,404,992-byte pointer array, instead of the page plus the doubled 16,384-byte array. `SpillSorterSuite` is already in the suite lists of `pr_build_linux.yml` and `pr_build_macos.yml` on `branch-1.0`, so CI runs the new test without a workflow change. ## Are there any user-facing changes? There are no config or API changes. When the JVM columnar shuffle's sorter grows its pointer array, it now doubles the array the way Spark does. It no longer asks for memory in proportion to its data pages. -- 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]
