Drop the *Agg suffix from all aggregate names#133
Open
estebanzimanyi wants to merge 3 commits into
Open
Conversation
DuckDB's catalog dispatches on argument types, so the same-stem collision that previously motivated the *Agg suffix (RFC #827, e.g. aggregate Tmin on tnumber vs scalar Tmin on tbox) is resolved by signature dispatch: the aggregate takes a temporal value, the scalar takes a box. The suffix was read as ugly by users and breaks the principle that the cross-platform SQL form should match what a Snowflake/Databricks/lakehouse programmer would naturally write. Rename across all 21 aggregates and their test references: TandAgg → Tand, TorAgg → Tor, TcountAgg → Tcount, TminAgg → Tmin, TmaxAgg → Tmax, TsumAgg → Tsum, TavgAgg → Tavg, TcentroidAgg → Tcentroid, MergeAgg → Merge, AppendInstantAgg → AppendInstant, AppendSequenceAgg → AppendSequence, SpanUnionAgg → SpanUnion, SetUnionAgg → SetUnion, WminAgg → Wmin, WmaxAgg → Wmax, WsumAgg → Wsum, WcountAgg → Wcount, WavgAgg → Wavg. C++ internal symbols (struct types like MergeAggFn, registration helpers) are unchanged where they don't appear in user-facing SQL. The static method SetTypes::RegisterSetUnionAgg is renamed to RegisterSetUnion for internal consistency with the renamed aggregate. Doc-comment cleanup in temporal_aggregates.cpp and the four affected parity test headers (031, 015, 040, 042) replaces references to RFC #827's Pascal-cased *Agg convention with the simpler explanation: DuckDB argument-type dispatch resolves collisions naturally. No semantic change to any aggregate; pure rename plus doc cleanup.
estebanzimanyi
force-pushed
the
feat/drop-agg-suffix
branch
2 times, most recently
from
6d36d04 to
ebd5ede
Compare
Member
Author
Reviewer's quickstart — ~2-3 minutesWhat this PR does: Drop the *Agg suffix from all aggregate names. Risk: focused, single-purpose. Spot-check the source diff + matching tests; CI confirms. Cross-link: Linux arm64 CI here needs #161 for the orthogonal |
`meosType` (lower-case) is the **pre-consolidation** MEOS type name; `MeosType` (upper-case) is the **post-consolidation** target that the upstream rename sweep has not yet reached. The current vcpkg pin (`vcpkg_ports/meos/portfile.cmake` REF f11b7443ee98…) is still pre-consolidation: `meos/include/temporal/meos_catalog.h` line 121 declares the typedef as `} meosType;` and every MEOS API uses the lower-case spelling. MobilityDuck's source code consistently uses `meosType` to match — `grep -rn '\bMeosType\b' src/` finds the name only on the alias line and its comment, nowhere else. c8cad6d added `using meosType = MeosType;` as a forward-looking bridge for the eventual consolidation bump. That bridge points at `MeosType`, which the current pin does NOT yet expose, so it breaks every PR's Linux arm64 build with: /duckdb_build_dir/src/include/tydef.hpp:18:18: error: ‘MeosType’ does not name a type; did you mean ‘meosType’? The fix is to drop the premature alias and replace the misleading comment with one that documents the pre/post-consolidation distinction and the resume path for the next pin bump — at that point a reviewer can either restore the bridge (this time it'll be valid because `MeosType` will exist) or sweep the MobilityDuck source from `meosType` to `MeosType` in a single PR. Unblocks every in-flight PR's Linux arm64 build: #126, #130, #149, #158, #159, #160, plus the entire `feat/*_port_core` extended-type stack (#148/#150/#151/#153/#155/#156).
The stage_icu helper mapped only the Linux uname values, so on the macOS arm64 test runner uname -m returned "arm64" and the icu extension was copied to .duckdb/extensions/v1.4.4/arm64 instead of .../osx_arm64, where DuckDB's autoload looks. The hub fallback is not reliably resolvable on that runner, so the osx_arm64 Test step failed to load the extension. Map the OS and architecture to the DuckDB platform string (linux_amd64, linux_arm64, osx_amd64, osx_arm64) so the locally built icu is staged at the path autoload expects on every tested platform; the Linux mapping is unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DuckDB's catalog dispatches on argument types, so the same-stem collision that previously motivated the *Agg suffix (RFC #827, e.g. aggregate Tmin on tnumber vs scalar Tmin on tbox) is resolved by signature dispatch: the aggregate takes a temporal value, the scalar takes a box, they coexist. The suffix read as ugly and broke the principle that the cross-platform SQL form should match what a Snowflake/Databricks/lakehouse programmer would naturally write. All 21 aggregates and their test references are renamed (TandAgg to Tand, TminAgg to Tmin, MergeAgg to Merge, WavgAgg to Wavg, etc.); doc comments in src/temporal/temporal_aggregates.cpp and the four affected parity test headers replace the *Agg-convention prose with the simpler argument-type-dispatch explanation. Pure rename plus doc cleanup, no semantic change. CI is currently blocked by an unrelated pre-existing vcpkg/MEOS pin issue on origin/main: portfile REF f11b7443e (pre-Apr-28 meosType to MeosType rename) does not match SHA512 ae8589acc8 (which corresponds to 742c1fb5, post-rename), so fresh downloads compile-fail at src/include/tydef.hpp:18 with "MeosType does not name a type"; a separate vcpkg-pin-fix PR is the right vehicle since fully bumping MEOS cascades into per-call signature updates beyond this rename scope.