Skip to content

feat(bindings): extent(tnumber) → tbox + bundle extent overloads in one set#49

Closed
estebanzimanyi wants to merge 1 commit into
feat/aggregate-fn-extent-spanset-setfrom
feat/aggregate-fn-extent-tnumber
Closed

feat(bindings): extent(tnumber) → tbox + bundle extent overloads in one set#49
estebanzimanyi wants to merge 1 commit into
feat/aggregate-fn-extent-spanset-setfrom
feat/aggregate-fn-extent-tnumber

Conversation

@estebanzimanyi

Copy link
Copy Markdown
Member

Summary

Adds 2 new aggregate overloads:

Overload Returns
`extent(tint)` `tbox` (TBOXINT XT)
`extent(tfloat)` `tbox` (TBOXFLOAT XT)

```sql
SELECT extent(t) FROM (VALUES (tint '[1@2000-01-01, 5@2000-01-05]'),
(tint '[10@2000-01-03, 20@2000-01-08]')) tt(t);
-- TBOXINT XT([1, 21),[2000-01-01 00:00:00+00, 2000-01-08 00:00:00+00])
```

Implementation

New module: `src/temporal/temporal_aggregates.cpp` + `src/include/temporal/temporal_aggregates.hpp`.

Component What it does
`TboxExtentState` Inline `TBox box` + `bool isset`. TBox is fixed-size POD (Span period + Span span + int16 flags).
`TnumberExtentFunction::Operation` Copies input Temporal blob into a heap allocation, calls MEOS `tnumber_extent_transfn`. First call returns a palloc'd TBox which we memcpy-and-free into state; subsequent calls expand state in place.
`TnumberExtentFunction::Combine` Merges two TBox states via `tbox_expand` — replicates the inline path in MEOS when both operands are non-null TBoxes.
`TnumberExtentFunction::Finalize` Returns blob or NULL.

API refactor (driven by adding the second module)

Previously each module registered its own `AggregateFunctionSet("extent")` directly via the loader. That worked when only `SpanAggregates` existed (PR #47). Adding `TemporalAggregates` with the same set name surfaced a DuckDB constraint: a second registration with the same aggregate name triggers an ALTER path, but `CreateAggregateFunctionInfo` doesn't override `GetAlterInfo()`, so it throws `NotImplementedException`.

Fix: both modules now expose `AddExtentOverloads(AggregateFunctionSet &)` that mutate a shared set. The load site in `LoadInternal` builds one set, calls both, then registers exactly once:

```cpp
AggregateFunctionSet extent_set("extent");
SpanAggregates::AddExtentOverloads(extent_set);
TemporalAggregates::AddExtentOverloads(extent_set);
loader.RegisterFunction(std::move(extent_set));
```

Stacked on

Test plan

…ne set

Adds two new aggregate overloads:

- extent(tint)   → tbox
- extent(tfloat) → tbox

Implementation in new module src/temporal/temporal_aggregates.cpp:
- TboxExtentState holds an inline TBox (period + value-span + flags),
  fixed-size POD.
- TnumberExtentFunction copies the input Temporal blob into a heap
  allocation before passing to MEOS tnumber_extent_transfn. On first
  call MEOS palloc's a fresh TBox; we memcpy into state and free.
  Subsequent calls expand the inline state via the same MEOS function.
- Combine merges two TBox states via tbox_expand (replicating MEOS's
  inline path when both operands are non-null).

Refactors how extent() overloads are registered: previously each module
registered its own AggregateFunctionSet("extent"), which broke when
two modules both did it — DuckDB rejects overlap-by-name registrations
because CreateAggregateFunctionInfo has no GetAlterInfo override.

Now both span_aggregates.cpp and temporal_aggregates.cpp expose
AddExtentOverloads(AggregateFunctionSet&) that mutate a shared set. The
load site in LoadInternal builds one set, calls both, then registers
once.
@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