Skip to content

feat(bindings): tAnd / tOr / tagg_min / tagg_max / tagg_sum aggregates#52

Closed
estebanzimanyi wants to merge 1 commit into
feat/aggregate-fn-tcountfrom
feat/aggregate-fn-tand-tor-tmin-tmax-tsum
Closed

feat(bindings): tAnd / tOr / tagg_min / tagg_max / tagg_sum aggregates#52
estebanzimanyi wants to merge 1 commit into
feat/aggregate-fn-tcountfrom
feat/aggregate-fn-tand-tor-tmin-tmax-tsum

Conversation

@estebanzimanyi

Copy link
Copy Markdown
Member

Summary

Adds 5 more skiplist-backed temporal aggregates on top of the infrastructure landed for `tCount` (PR #50):

Aggregate Inputs Output
`tAnd(tbool)` tbool tbool — per-instant boolean AND
`tOr(tbool)` tbool tbool — per-instant boolean OR
`tagg_min(tint)` / `tagg_min(tfloat)` tint / tfloat same — per-instant minimum
`tagg_max(tint)` / `tagg_max(tfloat)` tint / tfloat same — per-instant maximum
`tagg_sum(tint)` / `tagg_sum(tfloat)` tint / tfloat same — per-instant sum

ttext skipped — text merge needs `text_cmp` plumbing not yet wired.

```sql
SELECT tagg_min(t) FROM (VALUES (tint '5@2000-01-01'), (tint '3@2000-01-01')) tt(t);
-- {3@2000-01-01 00:00:00+00}

SELECT tAnd(t) FROM (VALUES (tbool 't@2000-01-01'), (tbool 'f@2000-01-01')) tt(t);
-- {f@2000-01-01 00:00:00+00}
```

Implementation

Factors the boilerplate from `TCountFunction` into a templated functor:

```cpp
template <TempTransfn TRANSFN, Datum (*MERGE_FN)(Datum, Datum), bool CROSSINGS>
struct SkiplistAggFunction { ... };
```

Each aggregate is now one `MakeSkiplistAggregate<&meos_transfn, &merge_fn, crossings>(input, output)` instantiation.

The merge functions used by `Combine` (`datum_and`, `datum_or`, `datum_min_int32`, `datum_max_int32`, `datum_sum_int32`, `datum_min_float8`, `datum_max_float8`, `datum_sum_float8`) are not exported from MEOS, so local equivalents are provided in this file. Float8 conversion goes through an aliased union to handle the IEEE 754 bit-pattern stored in `Datum`.

Naming note (`tagg_*` instead of `tMin`/`tMax`/`tSum`)

`Tmin` and `Tmax` are already registered as scalar time-min / time-max accessors on tbox / stbox (`src/temporal/tbox.cpp` line 396, 414, `src/geo/stbox.cpp` line 257, 302). DuckDB's catalog is case-insensitive at the name level, so registering an aggregate named `tMin` would lookup-collide with the existing scalar and trigger an `ALTER` path that `CreateAggregateFunctionInfo` doesn't implement (`GetAlterInfo not implemented for this type`) — extension fails to load.

The `tagg_*` prefix sidesteps the collision. A future cleanup could rename the existing scalars (e.g. to `time_min(tbox)` / `time_max(tbox)`) and reclaim the `tMin` / `tMax` / `tSum` names if MobilityDB-name parity is preferred.

Stacked on

Test plan

Adds 5 more skiplist-backed temporal aggregates on top of the
infrastructure landed for tCount:

  tAnd(tbool)      -> tbool   per-instant boolean AND
  tOr(tbool)       -> tbool   per-instant boolean OR
  tagg_min(tint)   -> tint    per-instant minimum
  tagg_min(tfloat) -> tfloat
  tagg_max(tint)   -> tint    per-instant maximum
  tagg_max(tfloat) -> tfloat
  tagg_sum(tint)   -> tint    per-instant sum
  tagg_sum(tfloat) -> tfloat

ttext skipped — text merge needs text_cmp plumbing not yet wired.

Implementation factors out the boilerplate from TCountFunction into
a templated SkiplistAggFunction<TRANSFN, MERGE_FN, CROSSINGS> functor.
Each aggregate is one MakeSkiplistAggregate<...> instantiation.

The merge functions used by Combine (datum_and, datum_or,
datum_min_int32, datum_max_int32, datum_sum_int32, datum_min_float8,
datum_max_float8, datum_sum_float8) are NOT exported from MEOS, so
local equivalents are provided. Float8 conversion goes through a
union to handle the IEEE 754 bit-pattern in Datum.

Naming note: tagg_min / tagg_max / tagg_sum instead of tMin / tMax /
tSum because Tmin / Tmax already exist as scalar time-min /
time-max accessors on tbox / stbox. DuckDB's catalog is
case-insensitive at the name level, so registering an aggregate
named tMin would trigger an ALTER path on the existing scalar that
CreateAggregateFunctionInfo doesn't implement, and the extension
would fail to load.
@estebanzimanyi

Copy link
Copy Markdown
Member Author

Consolidated into #60 (aggregate cluster squash). The full chain of aggregate work is now reviewable as a single PR; this branch's commits are preserved in #60's history. Closing to minimize the review queue.

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.

1 participant