apache / apache/datafusion-comet

Re-enable the two ignored CometAggregateSuite metric tests after the DataFusion 55 peak_mem_used change

Open
#5,703 2 comments 0 reactions 0 assignees View on GitHub
area:aggregation priority:medium requires-triage test
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

### Describe the bug

Two tests in `CometAggregateSuite` are disabled on the DataFusion 55 branch with only a prose TODO and no tracking issue:

```scala
// TODO: To be addressed after DF 55 migration
ignore("grouped aggregate metrics are forwarded without fabricating global metrics") {
...
// TODO: To be addressed after DF 55 migration
ignore("range sampling does not report grouped aggregate metrics") {
```

They assert on `spill_count`, `spilled_bytes`, `spilled_rows` and `peak_mem_used`. `peak_mem_used` accounting changed in apache/datafusion#22710, which is what broke them.

### Steps to reproduce

Un-`ignore` either test on the `df55` branch.

### Expected behavior

Either the assertions are adapted to DataFusion 55's accounting, or the metric is reported the way the tests expect. Either way the skips should point at this issue rather than a prose TODO, which is easy to lose once the branch merges.

### Additional context

@sunchao offered to take this as a follow-up after #5262 merges. Related: the `CometTPCHQuerySuite` off-heap bump tracked separately.

Contributor guide

Open the contributing guide

Research direction

Start with the two ignored tests in CometAggregateSuite on the df55 branch and review the DataFusion 55 peak_mem_used change referenced in apache/datafusion#22710. Run the suite with each test enabled, then adapt the assertions or metric reporting so spill_count, spilled_bytes, spilled_rows, and peak_mem_used match the intended behavior; replace the prose TODO with this issue reference.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust, scala, spark
Domain
data-engineering, testing-qa
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
68/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.