Repository navigation
Conversation
rluvaton
left a comment
There was a problem hiding this comment.
Left some thoughts, thank you for opening the PR!
| } | ||
| } | ||
|
|
||
| impl<T: Accumulator + ?Sized> ExplainMemory for T { |
There was a problem hiding this comment.
Wouldn't this will cause rust to disallow you to implement it on other trait.
As I wouldn't be able to do:
trait MyTrait {}
impl<T: MyTrait> ExplainMemory for T {
// ...
}As rust will say to me that MyTrait can implement Accumulator
There was a problem hiding this comment.
Hey @rluvaton! 👋
You're totally right to be cautious — that blanket impl of ExplainMemory for T where T: Accumulator will indeed block any other downstream impls of ExplainMemory for overlapping traits like T: MyTrait, due to Rust's coherence and orphan rules.
💡 Workaround: You can safely achieve what you want using a newtype pattern. That way, you avoid trait conflicts while still providing a blanket impl.
Here’s a minimal example:
// Newtype wrapper
struct AccumExplainer<T: Accumulator>(T);
// Blanket impl only applies to the wrapper, not all T
impl<T: Accumulator> ExplainMemory for AccumExplainer<T> {
fn explain(&self) -> String {
format!("Memory usage: {}", self.0.accumulate())
}
}
// Convenience (optional)
impl<T: Accumulator> From<T> for AccumExplainer<T> {
fn from(inner: T) -> Self {
Self(inner)
}
}You can now wrap any Accumulator type like this:
fn explain_it<T: Accumulator>(thing: T) {
let wrapped = AccumExplainer::from(thing);
println!("{}", wrapped.explain());
}This gives you a clean, conflict-free path to implement ExplainMemory broadly without locking out other impls for T in the future.
| /// ``` | ||
| pub trait ExplainMemory { | ||
| /// Returns a human readable string describing memory usage. | ||
| fn explain_memory(&self) -> Result<String>; |
There was a problem hiding this comment.
Do you think we should have something similar to formatter? So we will have easier to read memory report?
| /// reservation.try_grow(256).unwrap(); | ||
| /// println!("{}", reservation.explain_memory().unwrap()); | ||
| /// ``` | ||
| pub trait ExplainMemory { |
There was a problem hiding this comment.
I think it should include size as well so any trait that need to report the size will also can report the breakdown of the size.
|
closed by accident. |
|
Reopening.. |
Which issue does this PR close?
Rationale for this change
Datafusion’s memory management intentionally does not track every allocation. The MemoryPool docs explain that only “large” consumers (like grouping or joining) register allocations, while transient buffers are not tracked. This keeps memory accounting simpler and avoids overhead:
/// Rather than tracking all allocations, DataFusion takes a pragmatic approach:
/// Intermediate memory used as data streams through the system is not accounted
/// (it assumed to be "small") but the large consumers of memory must register
/// and constrain their use. This design trades off the additional code
/// complexity of memory tracking with limiting resource usage.
Therefore, DataFusion already has abstractions for tracking memory per consumer.
This PR leverages existing MemoryReservation sizes and report_top to implement debugging helpers that show how much memory each accumulator or structure uses by introducing a standardized way to inspect and debug memory usage across various components of the execution engine. The
ExplainMemorytrait provides a human-readable summary of memory usage, which is helpful for debugging and monitoring purposes. This also lays the groundwork for improving visibility into memory consumption patterns in complex queries.What changes are included in this PR?
memory_reportimplementing theExplainMemorytrait.ExplainMemoryfor anyAccumulatorusing itssize()method.ExplainMemoryforMemoryReservation.report_top_consumersthat attempts to downcast a memory pool and extract consumer memory usage stats.ExplainMemorytrait.ExplainMemoryforGroupedHashAggregateStreamto provide detailed insight into memory usage of group aggregates.lz4andzstdfeatures indatafusion/physical-planto allow test coverage of compression-related functionality.Are these changes tested?
Yes:
MemoryReservationandAccumulatorimplementations ofExplainMemory.GroupedHashAggregateStream's memory explanation logic compiles and integrates cleanly.Are there any user-facing changes?
Yes:
.explain_memory()on memory reservations and accumulators to obtain human-readable memory usage summaries.GroupedHashAggregateStream, aiding debugging and optimization efforts.