Skip to content

Conversation

@zachmu
Copy link
Member

@zachmu zachmu commented May 23, 2025

Other aggregate functions will require type overload resolution logic that this first function, array_agg, does not.

Also not yet supported: the ORDER BY clause in the function, e.g.:

array_agg ( anyarray ORDER BY input_sort_columns ) 

Relies on dolthub/go-mysql-server#2992

@zachmu zachmu requested a review from Hydrocharged May 23, 2025 23:45
@github-actions
Copy link
Contributor

github-actions bot commented May 23, 2025

Main PR
covering_index_scan_postgres 324.44/s 338.43/s +4.3%
index_join_postgres 156.42/s 158.46/s +1.3%
index_join_scan_postgres 187.07/s 189.55/s +1.3%
index_scan_postgres 12.53/s 12.61/s +0.6%
oltp_point_select 2546.25/s 2594.07/s +1.8%
oltp_read_only 1813.69/s 1840.77/s +1.4%
select_random_points 118.15/s 117.71/s -0.4%
select_random_ranges 130.98/s 130.81/s -0.2%
table_scan_postgres 11.80/s 11.74/s -0.6%
types_table_scan_postgres 5.44/s 5.47/s +0.5%

@github-actions
Copy link
Contributor

Main PR
Total 42090 42090
Successful 16437 16440
Failures 25653 25650
Partial Successes1 5549 5549
Main PR
Successful 39.0520% 39.0592%
Failures 60.9480% 60.9408%

${\color{lightgreen}Progressions (3)}$

arrays

QUERY: select array_agg(unique1) from (select unique1 from tenk1 where unique1 < 15 order by unique1) ss;
QUERY: select array_agg(ten) from (select ten from tenk1 where unique1 < 15 order by unique1) ss;
QUERY: select array_agg(unique1) from tenk1 where unique1 < -15;

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

Copy link
Collaborator

@Hydrocharged Hydrocharged left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My biggest questions revolve around how this extends off of GMS. Looking at the documentation, it looks like aggregate functions are composed of two internal functions. One that updates some internal state, and another that "finalizes" it. In all, that seems like a simpler abstraction than this, since it should work with all of the overload resolution logic and everything else. It would just be an extension on our existing CompiledFunction.

It's something we'll have to do anyway to support CREATE AGGREGATE, and it seems simpler overall, but perhaps you've already looked into this and it's not as straightforward as it seems, so would love to get your opinions on that.

@zachmu
Copy link
Member Author

zachmu commented May 27, 2025

My biggest questions revolve around how this extends off of GMS. Looking at the documentation, it looks like aggregate functions are composed of two internal functions. One that updates some internal state, and another that "finalizes" it. In all, that seems like a simpler abstraction than this, since it should work with all of the overload resolution logic and everything else. It would just be an extension on our existing CompiledFunction.

It's something we'll have to do anyway to support CREATE AGGREGATE, and it seems simpler overall, but perhaps you've already looked into this and it's not as straightforward as it seems, so would love to get your opinions on that.

My main thought is that I want to unify the aggregation planning between GMS and Doltgres. If we need to make changes to GMS to accommodate user-defined aggregations that's fine, but I don't think we need to re-write the GMS planning / execution just to get built-in postgres aggregates working. I think we can get a long way without supporting user-defined aggregates. I want to evolve this integration over time so we have a single aggregate abstraction, planning mode, and execution strategy driven by GMS. I can buy that postgres's internal way of updating aggregate func state is the right way forward, but I don't want to unify those models just yet.

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@zachmu zachmu merged commit be852fe into main May 28, 2025
14 checks passed
@zachmu zachmu deleted the zachmu/aggs branch May 28, 2025 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants