Skip to content

feat(bindings): full *Agg surface — cumulative aggregate cluster (consolidates #48–#59)#60

Closed
estebanzimanyi wants to merge 1 commit intomainfrom
feat/aggregate-fn-tcentroid
Closed

feat(bindings): full *Agg surface — cumulative aggregate cluster (consolidates #48–#59)#60
estebanzimanyi wants to merge 1 commit intomainfrom
feat/aggregate-fn-tcentroid

Conversation

@estebanzimanyi
Copy link
Copy Markdown
Member

Summary

Adds the temporal-centroid aggregate — per-instant geographic centroid across a column of tgeompoint values.

```sql
SELECT asText(tCentroid(t)) FROM (VALUES (tgeompoint 'Point(0 0)@2000-01-01'),
(tgeompoint 'Point(2 2)@2000-01-01')) tt(t);
-- {POINT(1 1)@2000-01-01 00:00:00+00}
```

Implementation

State holds a `SkipList` of `(sum_x, sum_y, count)` triples for 2D points or `(sum_x, sum_y, sum_z, count)` quadruples for 3D points.

Step What it does
`Operation` Calls `tpoint_tcentroid_transfn` which sets up the skiplist's `extra` field (`GeoAggregateState`: srid + hasz).
`Combine` When target is empty, takes ownership of source's skiplist (preserves `extra`). When both have data, splices via `mduck_datum_sum_double3` or `double4` as chosen by the source's `hasz` flag.
`Finalize` Calls `tpoint_tcentroid_finalfn` which divides each instant's `(sum_x, sum_y[, sum_z])` by `count` and frees the skiplist.

`datum_sum_double3` / `double4` and `GeoAggregateState` are not exported by MEOS public headers; their layouts are simple enough to replicate locally:

```cpp
struct mduck_double3 { double a, b, c; }; // (sum_x, sum_y, count)
struct mduck_double4 { double a, b, c, d; }; // (sum_x, sum_y, sum_z, count)
struct mduck_geo_agg_state { int32_t srid; bool hasz; };
```

Test plan

  • Single-row tCentroid returns the input point unchanged
  • Two-row same-instant centroid produces the midpoint correctly
  • No segfault on cleanup (destructor + finalfn double-free guard)

@nhungoc1508
Copy link
Copy Markdown
Collaborator

This PR has conflicts with the main branch due to divergence from #47. I'm not sure how to resolve the conflicts, especially in span_aggregates.cpp. Would you be able to help refreshing this PR? @estebanzimanyi

…x, tSum, tAvg, tCentroid, wavg, wmin/wmax/wsum, spanUnion

Lands the complete first-generation aggregate + window-aggregate surface
across numeric, boolean, text, tgeompoint, span, spanset and set types.

## Aggregates added

| Function | Input | Output |
|---|---|---|
| `tCount(temporal)` | any temporal | `tint` |
| `tAnd(tbool)` / `tOr(tbool)` | tbool | tbool |
| `tMin(T)` / `tMax(T)` | tnumber / ttext | same |
| `tSum(tnumber)` | tnumber | same |
| `tAvg(tnumber)` | tnumber | tfloat |
| `tCentroid(tgeompoint)` | tgeompoint | tgeompoint |
| `wMin/wMax/wSum(T, INTERVAL)` | tnumber | same |
| `wAvg(tnumber, INTERVAL)` | tnumber | tfloat |
| `spanUnion(span\|spanset)` | span/spanset | spanset |
| `extent(spanset\|set)` | span/spanset/set | span |

## Implementation

All skiplist-backed aggregates share `TemporalAggregates` in
`src/temporal/temporal_aggregates.cpp`.  The skiplist owns `TimestampTz→value`
instants; `Initialize` creates it, `Operation` calls the MEOS transfn,
`Combine` splices two skiplists via `tagg_combine`, `Finalize` calls the
MEOS finalfn and serialises the result blob.

`tCentroid` uses a `GeoAggregateState (srid + hasz)` extension stored in
the skiplist's `extra` field; the finalize path mirrors `tpoint_tcentroid_finalfn`.

`SpanAggregates` is refactored: `RegisterAggregateFunctions` is split into
`AddExtentOverloads` (for pooling into one extent set with temporal/spatial
overloads) and `RegisterSpanUnion`.  `LoadInternal` builds one unified
`AggregateFunctionSet extent` covering spans, tnumbers, and tgeompoints.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@estebanzimanyi estebanzimanyi force-pushed the feat/aggregate-fn-tcentroid branch from cfbe907 to 8fcd799 Compare May 4, 2026 10:07
@estebanzimanyi
Copy link
Copy Markdown
Member Author

Refreshed — the branch is now a single squashed commit on top of the current `main` tip (`deb692c`).

The conflict was an add/add: both sides added `span_aggregates.cpp` independently after the divergence point — main with the minimal `RegisterAggregateFunctions` API and this branch with the refactored `AddExtentOverloads + RegisterSpanUnion` API. Since the branch's version is a strict superset, I took it wholesale and discarded main's partial version.

GitHub confirms `"mergeable": "MERGEABLE"` — no conflicts remain.

@estebanzimanyi
Copy link
Copy Markdown
Member Author

Superseded by the consolidated PR branch consolidate/aggregates-parity. All changes from this PR are included in that branch as a single squashed commit. Please review and merge the consolidated branch instead.

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.

2 participants