Conversation
3da9184 to
532d434
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25126 +/- ##
========================================
Coverage 81.91% 81.91%
========================================
Files 1132 1132
Lines 421117 421273 +156
Branches 421117 421273 +156
========================================
+ Hits 344961 345102 +141
- Misses 55767 55771 +4
- Partials 20389 20400 +11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
003da84 to
b2db45f
Compare
| 02)--Projection: aggregate_test_100.c1, count(alias1) AS count(DISTINCT aggregate_test_100.c2), min(alias1) AS min(DISTINCT aggregate_test_100.c2), sum(alias2) AS sum(aggregate_test_100.c3), max(alias3) AS max(aggregate_test_100.c4) | ||
| 03)----Aggregate: groupBy=[[aggregate_test_100.c1]], aggr=[[count(alias1), min(alias1), sum(alias2), max(alias3)]] | ||
| 04)------Aggregate: groupBy=[[aggregate_test_100.c1, aggregate_test_100.c2 AS alias1]], aggr=[[sum(CAST(aggregate_test_100.c3 AS Int64)) AS alias2, max(aggregate_test_100.c4) AS alias3]] | ||
| 02)--Projection: aggregate_test_100.c1, count(alias1) AS count(DISTINCT aggregate_test_100.c2), min(alias2) AS min(DISTINCT aggregate_test_100.c2), sum(alias3) AS sum(aggregate_test_100.c3), max(alias4) AS max(aggregate_test_100.c4) |
There was a problem hiding this comment.
These changes are related due to an extra alias by this simplification.
|
@adriangb Would you have time for a review? Thanks a lot! |
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing min-max-distinct (db160ce) to 0015a75 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing min-max-distinct (db160ce) to 0015a75 (merge-base) diff Run configurationrun benchmark tpcdsResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing min-max-distinct (db160ce) to 0015a75 (merge-base) diff Run configurationrun benchmark clickbench_partitionedResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing min-max-distinct (db160ce) to 0015a75 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing min-max-distinct (db160ce) to 0015a75 (merge-base) diff Run configurationrun benchmark tpcdsCPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing min-max-distinct (db160ce) to 0015a75 (merge-base) diff Run configurationrun benchmark clickbench_partitionedCPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
The changes in |
db160ce to
ca5f5dd
Compare
ca5f5dd to
5b8a629
Compare
|
Wouldn't this work for max as well? |
It looks like @neilconway predicted this one: #22644. It sounds like what we need is A trait method on
|
+1 for this |
|
Thanks for the feedback! |
|
Superseeded by #25288 |
Which issue does this PR close?
Rationale for this change
Simplify
min(DISTINCT x)tomin(x)in a grouped query makes the query need 1000x less memory on the following setup:SELECT g, min(DISTINCT x) FROM t GROUP BY gSELECT g, min(x) FROM t GROUP BY gThe aggregation function
minis duplicate insensitive, therefore we can do the following simplification:min(DISTINCT x)is identicalmin(x)This also prevents that the optimizer rule
SingleDistinctToGroupByfires which leads to a more efficient plan:Before this change:
after:
Running
SELECT g, min(DISTINCT x) FROM t GROUP BY gagainst both branches:The query is roughly 11x faster with 1000x less memory usage.
DuckDB is following the same approach:
What changes are included in this PR?
min(distinct x)tomin(x)SingleDistinctToGroupByWhat is the testing strategy for this PR?
Are there any user-facing changes?
More memory efficient queries