ErikBPF commented on issue #5703: URL: https://github.com/apache/datafusion-comet/issues/5703#issuecomment-5655201002
I reproduced the two ignored aggregate-metric tests on `f69c4c81b9429e327ea95658530ae4ed4ed19635`, with DataFusion 55.1.0, Spark 4.1.3, and a release native library. I re-enabled only those tests and explicitly enabled `CometConf.COMET_RESPECT_DATAFUSION_CONFIGS` in each test's configuration. The sole difference between the two diagnostic runs was `spark.comet.datafusion.execution.enable_migration_aggregate`: | Value | Result | | --- | --- | | `true` (default) | Both tests failed at positive `peak_mem_used` assertions | | `false` | Both tests passed | The grouped test reported `Expected peak aggregate memory for modes List(Final)`. The sampling test passed the sampling-exclusion checks and failed at the positive peak assertion after execution. Source inspection matches this result: the migrated implementation in DataFusion's `aggregates/hash_stream.rs` uses memory reservations and spill metrics but does not register the `peak_mem_used` gauge. The legacy grouped stream registers and updates it; Comet forwards registered metrics. This points to missing peak-memory instrumentation in migrated aggregation. It does not show missing memory accounting or prove increased memory use. Disabling migration was only a diagnostic control. I have left the tests and production defaults unchanged, since restoration of these metrics is already being coordinated. -- 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]
