2010YOUY01 commented on issue #24768:
URL: https://github.com/apache/datafusion/issues/24768#issuecomment-6126086240
I've been thinking about this recently, and here are some ideas.
In summary, I recommend implementing spilling in a separate stream rather
than extending the existing implementation. I'll help with some infrastructure
work to make this happen. Here's the full rationale:
## General issues with spilling
I've observed a concerning pattern: over the past 2–3 years, the more
spilling-related bugs we fix, the more bugs seem to surface later. (It would be
interesting to collect some actual numbers on bug reports and fixes.)
I think there are 3 potential reasons:
1. **The spilling protocol itself may be flawed.** Our memory pool
implementations (e.g., `FairSpillPool`) provide a protocol for reserving
memory. Perhaps the protocol itself contains logic bugs, or perhaps it's
underspecified, allowing operator implementations to drift from the protocol
without us noticing.
- This is still a very rough intuition based on some existing bug
reports. I'll try to formulate it better with concrete examples.
2. **Lack of behavioral (end-to-end) tests.** Testing this feature should be
fairly straightforward: run a query with a 1 GB memory pool and measure actual
RSS against an expected bound, rather than relying only on unit tests that
probe what `MemoryPool` reports.
- And we have very few such behavioral tests to guard against regressions.
3. **Convoluted implementations inside operators**, making it difficult to
reason about memory reservation logic and fix bugs correctly.
- At least right now, I can't easily understand the HJ logic. The
concerning part is that anyone can push AI to keep adding new features to the
existing implementation, making it even harder to maintain. (My imaginary
failure mode is, after exceeding some complexity threshold, AI always fix one
bug by bringing in two new bugs)
Right now, I'm focusing on point 3
-
[https://github.com/apache/datafusion/issues/26225](https://github.com/apache/datafusion/issues/26225)
-
[https://github.com/apache/datafusion/pull/26227](https://github.com/apache/datafusion/pull/26227)
These 3 reasons seem to be fairly deep-rooted issues. Addressing them is
more of a bottom-up approach, while directly implementing spilling is a
top-down approach. I think both efforts can progress concurrently if we
introduce a dedicated experimental HJ stream.
The reasons are:
- We don't really know how to make spilling work reliably yet. Even simpler
operators still have stability issues with spilling, so it's likely that
implementing spilling for a more complex HJ will run into similar problems.
- The join spilling algorithm itself is tricky. I don't see a clear
long-term path yet, and experimenting back and forth on the main execution path
could make other features harder to land.
So perhaps we could implement a dedicated `SpillingHashJoinStream`, mark it
as an experimental feature, and use a configuration option to turn it on or
off. Once we've made enough progress, we can port it back to the main execution
stream.
Development should also be smoother: since it's experimental, PRs could be
merged quickly once all major participants have had a chance to respond.
To start, perhaps we can fork the implementation in #26227 and implement
spilling for regular joins only. You could even prune the implementation
further to simplify it.
--
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]
