2010YOUY01 commented on issue #22723: URL: https://github.com/apache/datafusion/issues/22723#issuecomment-4632466216
I don't think the error message is the key issue. The problem is that this direction enforces an invariant that does not exist today. More specifically, the bug reported by this issue — internal accounting showing 150 KB vs. actual allocation of 770 KB — is arguably a feature rather than a bug under the current convention. See: * https://github.com/apache/datafusion/blob/84bc8761ac3a126e41658b6cd0ec6bd8cc34cda8/datafusion/execution/src/memory_pool/mod.rs#L60-L69 * https://github.com/apache/datafusion/issues/22723#issuecomment-4618433648 I think it is fair to say that this convention may be a bad design, and I am +1 on exploring a direction to fix it. However, I do not think we should write tests that assume this convention has already been invalidated. In particular, enabling this `slt` check would effectively assert that `MemoryPool` accounting must closely track actual allocator usage. That is inconsistent with both the current implementation and the documented convention, where the memory pool intentionally accounts for selected operator-managed memory rather than every byte actually allocated. So the test would not just catch regressions; it would implicitly change the contract by assuming a new accounting convention. If we can agree on that, I am happy to help provide pointers for exploring an alternative memory-accounting convention, and eventually enabling this `slt` validation once the new convention is defined. -- 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]
