search

timescale / timescaledb

An open-source time-series SQL database optimized for fast ingest and complex queries. Packaged as a PostgreSQL extension.
https://www.timescale.com/
Other
16.8k stars 852 forks source link

Remove ts_plan_add_hashagg #7077

Open fabriziomello opened 4 days ago

codecov[bot] commented 4 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 81.78%. Comparing base (59f50f2) to head (ab16f70). Report is 232 commits behind head on main.

:exclamation: Current head ab16f70 differs from pull request most recent head 010d0c6

Please upload reports for the commit 010d0c6 to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #7077 +/- ## ========================================== + Coverage 80.06% 81.78% +1.71% ========================================== Files 190 200 +10 Lines 37181 37350 +169 Branches 9450 9740 +290 ========================================== + Hits 29770 30547 +777 + Misses 2997 2887 -110 + Partials 4414 3916 -498 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

akuzm commented 4 days ago

Discovered this one recently as well. I had to move it before the chunkwise aggregation, or else it would lead to some very weird planning effects: chunkwise aggregation is used unconditionally and removes all the old paths, but after that we make a cost-based decision about these hash agg paths. This change is not merged yet.

Do you think we don't need it already? I can imagine it has value for vectorized aggregation, because currently Partial GroupAggregate cannot be vectorized, only the Partial HashAggregate. Would be interesting to look at the plan changes.

fabriziomello commented 20 hours ago

Discovered this one recently as well. I had to move it before the chunkwise aggregation, or else it would lead to some very weird planning effects: chunkwise aggregation is used unconditionally and removes all the old paths, but after that we make a cost-based decision about these hash agg paths. This change is not merged yet.

Do you think we don't need it already? I can imagine it has value for vectorized aggregation, because currently Partial GroupAggregate cannot be vectorized, only the Partial HashAggregate. Would be interesting to look at the plan changes.

I'm not a planner expert but this is an attempt after a pvt conversation with @svenklemm ... the motivation of this test is to fix this SDC: https://github.com/timescale/Support-Dev-Collab/issues/1859