jayzhan211 commented on issue #25650: URL: https://github.com/apache/datafusion/issues/25650#issuecomment-6392665249
I opened #26728 as a draft. It adds a test that fails when an operator holds memory it never reserved, which is the cause of the OOM kills this issue is about. The memory limit only covers what operators reserve. When an operator holds memory without reserving it, the pool still reports room, nothing spills, and the process can be killed even with a limit set. CI has no test for this in most operators: `memory_limit_validation` compares RSS with hand-picked bounds for sort and two joins only, and the drift logging from #25651 only logs. So these bugs are found one report at a time, and a fixed one can come back without anyone noticing. Requiring allocated memory to equal reserved memory is too strict. The `MemoryPool` docs let batches in flight go unreserved, and that is why #22626 was reverted (https://github.com/apache/datafusion/issues/22723#issuecomment-4632466216). What the docs do require is that memory which grows with the input is reserved, and the PR checks exactly that. Each case in `memory_limit_validation` already runs on 10% of the data and on all of it. A case now fails if unreserved memory grows by 16 MB or more between the two runs. This is a real problem today. On `main`, six common queries fail the check, each holding 120–590 MB more unreserved memory at 10M rows than at 1M: - the sort merge (#26411) - `median` and `array_agg` across partitions (#26705) - aggregate output (#26701) - `DISTINCT` (#25606) - windows (#26707) They are `#[ignore]`d with their issues, so each fix removes its `#[ignore]` and the test keeps that bug from coming back. TopK, a 10-group aggregate, hash joins and the existing cases pass. Following https://github.com/apache/datafusion/issues/25650#issuecomment-5805379639, the PR extends the existing suite instead of adding a second one. It also moves the counting allocator from #25651 into `test-utils`, so both suites share one copy. Before I mark it ready, I'd like to confirm two things: that extending the existing suite this way is the unification you had in mind, and that moving the allocator out of `datafusion-sqllogictest` is OK. -- 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]
