Support `GroupsAccumulator` for Avg duration #15748

shruti2522 · 2025-04-17T12:02:25Z

Which issue does this PR close?

Closes Support AVG for durations #15458 .

Rationale for this change

What changes are included in this PR?

Implement GroupsAccumulator for avg duration calculation

Are these changes tested?

Yes

Are there any user-facing changes?

Yes

shruti2522 · 2025-04-21T07:07:53Z

Ready for review @alamb

goldmedal

Thanks @shruti2522. I found that the tests will be passed in the current main branch. Could you double-check if we still need this change?

goldmedal · 2025-04-28T12:47:46Z

datafusion/sqllogictest/test_files/aggregate.slt

+# avg_duration (GroupsAccumulator)
+
+statement ok
+create table duration as values
+  (arrow_cast(10, 'Duration(Second)'), arrow_cast(100, 'Duration(Millisecond)'), 'a', 1),
+  (arrow_cast(20, 'Duration(Second)'), arrow_cast(200, 'Duration(Millisecond)'), 'a', 2),
+  (arrow_cast(30, 'Duration(Second)'), arrow_cast(300, 'Duration(Millisecond)'), 'b', 1),
+  (arrow_cast(40, 'Duration(Second)'), arrow_cast(400, 'Duration(Millisecond)'), 'b', 2),
+  (arrow_cast(50, 'Duration(Second)'), arrow_cast(500, 'Duration(Millisecond)'), 'c', 1),
+  (arrow_cast(60, 'Duration(Second)'), arrow_cast(600, 'Duration(Millisecond)'), 'c', 2);
+
+query T??I
+SELECT column3, avg(column1), avg(column2), column4 FROM duration GROUP BY column3, column4 ORDER BY column3, column4;


Actually, the following tests will be passed after #15468 in the current main branch.

alamb · 2025-04-28T15:57:39Z

I think the groups accumulator will result in faster performance, not new functionality.

@shruti2522 maybe you can verify this with datafusion-cli and using generate_series using a duration column

goldmedal · 2025-04-29T12:31:04Z

I think the groups accumulator will result in faster performance, not new functionality.

Thanks for the explanation. If it's a performance improvement, it's better to run some benchmark or add the benchmark case in datafusion/functions-aggregate/benches to prove the result.

shruti2522 · 2025-04-30T01:18:47Z

Got it @alamb @goldmedal, will test and share results soon.

alamb · 2025-05-05T19:32:02Z

I wrote up a ticket about how to make a benchmark:

Add Extended Clickbench benchmark for avg(duration) #15949

shruti2522 · 2025-05-05T19:47:05Z

I wrote up a ticket about how to make a benchmark:

Add Extended Clickbench benchmark for avg(duration) #15949

Thanks, @alamb. I've been a bit caught up with semester exams lately, but I’ll get it done by tomorrow.

alamb · 2025-05-05T20:06:49Z

I wrote up a ticket about how to make a benchmark:

Add Extended Clickbench benchmark for avg(duration) #15949

Thanks, @alamb. I've been a bit caught up with semester exams lately, but I’ll get it done by tomorrow.

Thank you so much! I totally re: school (which should take top priority for sure)

alamb · 2025-05-19T18:08:35Z

@logan-keede just finished up a benchmark for this PR here;

minor: Add benchmark query and corresponding documentation for Average Duration #16105

I will merge this PR up from main and run the benchmarks on it to see if we can see the expected improvement

alamb · 2025-05-19T18:31:09Z

🤖 ./gh_compare_branch.sh Benchmark Script Running
Linux aal-dev 6.11.0-1013-gcp #13~24.04.1-Ubuntu SMP Wed Apr 2 16:34:16 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing avg_duration_ga (d0c8e5c) to 62fa67c diff
Benchmarks: tpch_mem clickbench_partitioned clickbench_extended
Results will be posted here when complete

alamb · 2025-05-19T19:14:31Z

🤖: Benchmark completed

Details

Comparing HEAD and avg_duration_ga
--------------------
Benchmark clickbench_extended.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃       HEAD ┃ avg_duration_ga ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0     │  1936.41ms │       1947.22ms │     no change │
│ QQuery 1     │   699.84ms │        740.64ms │  1.06x slower │
│ QQuery 2     │  1421.42ms │       1445.72ms │     no change │
│ QQuery 3     │   697.75ms │        682.87ms │     no change │
│ QQuery 4     │  1480.14ms │       1481.14ms │     no change │
│ QQuery 5     │ 15403.25ms │      15432.33ms │     no change │
│ QQuery 6     │  2046.41ms │       2051.75ms │     no change │
│ QQuery 7     │  2107.72ms │       2081.03ms │     no change │
│ QQuery 8     │  2029.66ms │        816.71ms │ +2.49x faster │
└──────────────┴────────────┴─────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary              ┃            ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (HEAD)              │ 27822.62ms │
│ Total Time (avg_duration_ga)   │ 26679.42ms │
│ Average Time (HEAD)            │  3091.40ms │
│ Average Time (avg_duration_ga) │  2964.38ms │
│ Queries Faster                 │          1 │
│ Queries Slower                 │          1 │
│ Queries with No Change         │          7 │
└────────────────────────────────┴────────────┘
--------------------
Benchmark clickbench_partitioned.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━┓
┃ Query        ┃       HEAD ┃ avg_duration_ga ┃       Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━┩
│ QQuery 0     │     2.28ms │          2.50ms │ 1.10x slower │
│ QQuery 1     │    33.93ms │         33.71ms │    no change │
│ QQuery 2     │    80.31ms │         80.78ms │    no change │
│ QQuery 3     │    96.94ms │         96.88ms │    no change │
│ QQuery 4     │   589.62ms │        671.68ms │ 1.14x slower │
│ QQuery 5     │   866.95ms │        905.71ms │    no change │
│ QQuery 6     │     2.21ms │          2.31ms │    no change │
│ QQuery 7     │    38.15ms │         40.66ms │ 1.07x slower │
│ QQuery 8     │   932.57ms │        923.88ms │    no change │
│ QQuery 9     │  1188.77ms │       1235.34ms │    no change │
│ QQuery 10    │   267.04ms │        267.39ms │    no change │
│ QQuery 11    │   306.02ms │        301.30ms │    no change │
│ QQuery 12    │   926.06ms │        907.65ms │    no change │
│ QQuery 13    │  1350.50ms │       1341.27ms │    no change │
│ QQuery 14    │   851.07ms │        852.77ms │    no change │
│ QQuery 15    │   830.80ms │        827.94ms │    no change │
│ QQuery 16    │  1725.02ms │       1736.16ms │    no change │
│ QQuery 17    │  1593.49ms │       1604.16ms │    no change │
│ QQuery 18    │  3072.68ms │       3110.02ms │    no change │
│ QQuery 19    │    85.41ms │         84.68ms │    no change │
│ QQuery 20    │  1159.48ms │       1122.72ms │    no change │
│ QQuery 21    │  1310.66ms │       1326.77ms │    no change │
│ QQuery 22    │  2243.94ms │       2195.34ms │    no change │
│ QQuery 23    │  8117.51ms │       8119.91ms │    no change │
│ QQuery 24    │   472.97ms │        468.67ms │    no change │
│ QQuery 25    │   394.08ms │        389.20ms │    no change │
│ QQuery 26    │   531.17ms │        537.46ms │    no change │
│ QQuery 27    │  1603.15ms │       1598.93ms │    no change │
│ QQuery 28    │ 12635.59ms │      12436.73ms │    no change │
│ QQuery 29    │   523.31ms │        535.92ms │    no change │
│ QQuery 30    │   802.89ms │        813.79ms │    no change │
│ QQuery 31    │   829.54ms │        879.18ms │ 1.06x slower │
│ QQuery 32    │  2666.22ms │       2695.03ms │    no change │
│ QQuery 33    │  3357.57ms │       3389.70ms │    no change │
│ QQuery 34    │  3380.93ms │       3390.82ms │    no change │
│ QQuery 35    │  1306.60ms │       1319.62ms │    no change │
│ QQuery 36    │   124.06ms │        124.41ms │    no change │
│ QQuery 37    │    55.20ms │         59.42ms │ 1.08x slower │
│ QQuery 38    │   123.32ms │        124.62ms │    no change │
│ QQuery 39    │   198.12ms │        197.25ms │    no change │
│ QQuery 40    │    47.80ms │         47.60ms │    no change │
│ QQuery 41    │    44.94ms │         46.92ms │    no change │
│ QQuery 42    │    38.79ms │         39.10ms │    no change │
└──────────────┴────────────┴─────────────────┴──────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓
┃ Benchmark Summary              ┃            ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━┩
│ Total Time (HEAD)              │ 56807.69ms │
│ Total Time (avg_duration_ga)   │ 56885.88ms │
│ Average Time (HEAD)            │  1321.11ms │
│ Average Time (avg_duration_ga) │  1322.93ms │
│ Queries Faster                 │          0 │
│ Queries Slower                 │          5 │
│ Queries with No Change         │         38 │
└────────────────────────────────┴────────────┘
--------------------
Benchmark tpch_mem_sf1.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃     HEAD ┃ avg_duration_ga ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 1     │ 123.70ms │        122.14ms │     no change │
│ QQuery 2     │  23.77ms │         23.88ms │     no change │
│ QQuery 3     │  35.07ms │         35.23ms │     no change │
│ QQuery 4     │  20.50ms │         20.94ms │     no change │
│ QQuery 5     │  55.65ms │         55.57ms │     no change │
│ QQuery 6     │  12.26ms │         12.35ms │     no change │
│ QQuery 7     │ 106.78ms │        103.52ms │     no change │
│ QQuery 8     │  25.23ms │         27.02ms │  1.07x slower │
│ QQuery 9     │  64.04ms │         63.50ms │     no change │
│ QQuery 10    │  58.51ms │         59.24ms │     no change │
│ QQuery 11    │  12.48ms │         12.91ms │     no change │
│ QQuery 12    │  45.54ms │         46.57ms │     no change │
│ QQuery 13    │  30.39ms │         31.13ms │     no change │
│ QQuery 14    │  10.09ms │         10.05ms │     no change │
│ QQuery 15    │  25.09ms │         24.85ms │     no change │
│ QQuery 16    │  23.56ms │         22.85ms │     no change │
│ QQuery 17    │ 101.35ms │        101.06ms │     no change │
│ QQuery 18    │ 248.06ms │        231.39ms │ +1.07x faster │
│ QQuery 19    │  27.39ms │         27.56ms │     no change │
│ QQuery 20    │  38.15ms │         38.13ms │     no change │
│ QQuery 21    │ 169.82ms │        173.86ms │     no change │
│ QQuery 22    │  17.95ms │         17.58ms │     no change │
└──────────────┴──────────┴─────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━┓
┃ Benchmark Summary              ┃           ┃
┡━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━┩
│ Total Time (HEAD)              │ 1275.36ms │
│ Total Time (avg_duration_ga)   │ 1261.31ms │
│ Average Time (HEAD)            │   57.97ms │
│ Average Time (avg_duration_ga) │   57.33ms │
│ Queries Faster                 │         1 │
│ Queries Slower                 │         1 │
│ Queries with No Change         │        20 │
└────────────────────────────────┴───────────┘

alamb · 2025-05-20T11:13:41Z

--------------------
Benchmark clickbench_extended.json
--------------------
┏━━━━━━━━━━━━━━┳━━━━━━━━━━━━┳━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━━━━┓
┃ Query        ┃       HEAD ┃ avg_duration_ga ┃        Change ┃
┡━━━━━━━━━━━━━━╇━━━━━━━━━━━━╇━━━━━━━━━━━━━━━━━╇━━━━━━━━━━━━━━━┩
│ QQuery 0     │  1936.41ms │       1947.22ms │     no change │
│ QQuery 1     │   699.84ms │        740.64ms │  1.06x slower │
│ QQuery 2     │  1421.42ms │       1445.72ms │     no change │
│ QQuery 3     │   697.75ms │        682.87ms │     no change │
│ QQuery 4     │  1480.14ms │       1481.14ms │     no change │
│ QQuery 5     │ 15403.25ms │      15432.33ms │     no change │
│ QQuery 6     │  2046.41ms │       2051.75ms │     no change │
│ QQuery 7     │  2107.72ms │       2081.03ms │     no change │
│ QQuery 8     │  2029.66ms │        816.71ms │ +2.49x faster │ <--- A pretty nice result!
└──────────────┴────────────┴─────────────────┴───────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┳━━━━━━━━━━━━┓

alamb

I think this PR is now looking good thanks to @shruti2522 @goldmedal and @logan-keede

I merged up from main to resolve a conflict

I also added support for Duration to the fuzz tester in a separate PR to increase coverage:

Test Duration in aggregation fuzz tests #16111

logan-keede · 2025-05-20T13:20:35Z

QQuery 4 │ 589.62ms │ 671.68ms │ 1.14x slower <----

is this a cause of concern?

alamb · 2025-05-20T13:25:34Z

│ QQuery 4 │ 589.62ms │ 671.68ms │ 1.14x slower │

Given Q4 seems to be unrelated to avg:
https://github.com/apache/datafusion/blob/1a3917545c34e162272af9200da12860744c1abd/benchmarks/queries/clickbench/queries.sql#L5-L4

SELECT COUNT(DISTINCT "UserID") FROM hits;

I am assuming this was noise in the measurements

alamb · 2025-05-21T11:25:35Z

🚀

alamb · 2025-05-21T11:25:48Z

Thanks again @logan-keede and @shruti2522 -- this is a very nice improvement I think

github-actions bot added sqllogictest SQL Logic Tests (.slt) functions Changes to functions implementation labels Apr 17, 2025

shruti2522 force-pushed the avg_duration_ga branch from d833b56 to 2506180 Compare April 20, 2025 15:15

Support GroupsAccumulator for avg duration

2f07061

shruti2522 force-pushed the avg_duration_ga branch from 293be12 to 2f07061 Compare April 20, 2025 22:51

shruti2522 marked this pull request as ready for review April 20, 2025 22:52

goldmedal reviewed Apr 28, 2025

View reviewed changes

update test

102c809

alamb mentioned this pull request May 5, 2025

Add Extended Clickbench benchmark for avg(duration) #15949

Closed

alamb mentioned this pull request May 19, 2025

minor: Add benchmark query and corresponding documentation for Average Duration #16105

Merged

Merge remote-tracking branch 'apache/main' into avg_duration_ga

d0c8e5c

Merge remote-tracking branch 'apache/main' into avg_duration_ga

0dc0d54

alamb mentioned this pull request May 20, 2025

Test Duration in aggregation fuzz tests #16111

Merged

alamb approved these changes May 20, 2025

View reviewed changes

zhuqi-lucas approved these changes May 20, 2025

View reviewed changes

alamb force-pushed the avg_duration_ga branch from eb2ab94 to 0dc0d54 Compare May 20, 2025 13:23

alamb mentioned this pull request May 21, 2025

Minor: Add Accumulator::return_type and StateFieldsArgs::return_type to help with upgrade to 48 #16112

Merged

alamb merged commit 52f340b into apache:main May 21, 2025
53 of 54 checks passed

alamb added the performance Make DataFusion faster label May 21, 2025

Support GroupsAccumulator for Avg duration #15748

Support GroupsAccumulator for Avg duration #15748

Uh oh!

Conversation

shruti2522 commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

shruti2522 commented Apr 21, 2025

Uh oh!

goldmedal left a comment

Choose a reason for hiding this comment

Uh oh!

goldmedal Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 28, 2025

Uh oh!

goldmedal commented Apr 29, 2025

Uh oh!

shruti2522 commented Apr 30, 2025

Uh oh!

alamb commented May 5, 2025

Uh oh!

shruti2522 commented May 5, 2025

Uh oh!

alamb commented May 5, 2025

Uh oh!

alamb commented May 19, 2025

Uh oh!

alamb commented May 19, 2025

Uh oh!

alamb commented May 19, 2025

Uh oh!

alamb commented May 20, 2025

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

logan-keede commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alamb commented May 20, 2025

Uh oh!

alamb commented May 21, 2025

Uh oh!

Uh oh!

alamb commented May 21, 2025

Uh oh!

Uh oh!

Support `GroupsAccumulator` for Avg duration #15748

Support `GroupsAccumulator` for Avg duration #15748

shruti2522 commented Apr 17, 2025 •

edited

Loading

logan-keede commented May 20, 2025 •

edited

Loading