EmilyMatt commented on PR #15613: URL: https://github.com/apache/datafusion/pull/15613#issuecomment-2783287013
> Thank you @EmilyMatt -- this API make a lot of sense to me > > I think it would be useful to add tests for this feature, so that we don't break it in the future accidentally > > Specifically, I think testing the "memory consumer ids are unique" behavior would be best as that is the goal of the API. SO for example, perhaps write a query that has multiple `AggregateExec`s and verify that the assigned memory consumer Ids are different > > I would also recommend adding ids to the various messages (such as in TrackedConsumerPool) https://github.com/apache/datafusion/blob/main/datafusion/execution/src/memory_pool/pool.rs#L261 > > So that the id can be correlated in more places I have modified this PR to also reflect these changes in the TrackConsumer pool, but in order to ensure a valid state this also requires a breaking API change - MemoryConsumer no longer implements Hash and Clone This is in order to ensure a user does not call clone() on a consumer, and then register the cloned one, while still maintaining an owned MemoryConsumer, which can be called with register() again, despite having the same id as an already existing MemoryConsumer in the pool. I'd have preferred to deprecate the Clone implementation and only removing it in a future version, along with an explanation, but unfortunately, traits cannot be deprecated using the attribute. This means this PR needs the api change label, but I can't manually set 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: github-unsubscr...@datafusion.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org For additional commands, e-mail: github-h...@datafusion.apache.org
