From b5ee4ed43cebc878ed69e3b736a3575546a59b79 Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Sun, 10 May 2026 17:15:09 +0200 Subject: [PATCH 1/5] doc: PR Reviewer Guide + visibility wiring + ecosystem consolidation policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Adds `doc/contributing/reviewer-guide.md` — single source of truth for reviewing open PRs. Canonical structure shared with MobilityDB (PR #931) and MobilitySpark (PR #8): How to find this guide → CI legend (5 symbols incl. ⚠️) → Dependency chains → Tier 1/2/3 → Review checklist. - Wires visibility for reviewers landing on the repo: - `.github/PULL_REQUEST_TEMPLATE.md` — banner appears in every new PR. - `README.md` — "For contributors and reviewers" section, links to the reviewer guide; mentions canonical cross-repo structure. - Establishes the rule: every PR commit that changes queue state (opens/closes PR, discovers a dependency) updates the guide in the same commit. Once this lands and companion PRs land on MobilityDB/MobilitySpark, anyone landing in any of the three platform repos finds the same canonical guide at the same path with consistent visibility through the README and the PR template. --- .github/PULL_REQUEST_TEMPLATE.md | 14 ++++ README.md | 8 ++ doc/contributing/reviewer-guide.md | 119 +++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+) create mode 100644 .github/PULL_REQUEST_TEMPLATE.md create mode 100644 doc/contributing/reviewer-guide.md diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md new file mode 100644 index 00000000..6275a346 --- /dev/null +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -0,0 +1,14 @@ + + +## Summary + + + +## Test plan + + diff --git a/README.md b/README.md index 0b8c97b5..058d4362 100644 --- a/README.md +++ b/README.md @@ -21,6 +21,14 @@ MobilityDuck because of properties of DuckDB's parser, type system, or extension model. These cases — and the named-function workarounds where one exists — are documented in [`docs/DuckDB-Parity-Gaps.md`](docs/DuckDB-Parity-Gaps.md). +### For contributors and reviewers + +- Reviewing a pull request? See the + [PR Reviewer Guide](doc/contributing/reviewer-guide.md) — tier ranking, + dependency chains and the standards checklist. Reviewers landing in + any of the three platform repos (MobilityDB / MobilityDuck / + MobilitySpark) find the same canonical structure at the same path. + --- ## 1. Requirements MobilityDuck needs some dependencies(including MEOS) which can be installed through VCPKG. Run the following to enable it: diff --git a/doc/contributing/reviewer-guide.md b/doc/contributing/reviewer-guide.md new file mode 100644 index 00000000..11cc0862 --- /dev/null +++ b/doc/contributing/reviewer-guide.md @@ -0,0 +1,119 @@ + + +# MobilityDuck PR Reviewer Guide + +Quick reference for anyone reviewing open pull requests. +Updated in the same commit as any PR that changes PR state or adds new branches. +**Last updated: 2026-05-10 — 22 open PRs (net consolidation today): folds #115+#119 → #120, #116+#118 → #121, #122+#123+#124+#125 → #126. Squashed in place: #117, #111. PR #112 TZ-neutral test fix landed (1 commit).** + +--- + +## How to find this guide + +- **In the repo:** `doc/contributing/reviewer-guide.md` +- **Rule:** every commit that opens, closes, or restructures a PR must update this file in the same commit. A one-liner status change is enough; a fuller rewrite is needed when the dependency graph changes. + +--- + +## CI legend + +| Symbol | Meaning | +|--------|---------| +| ✅ | All checks green | +| ❌ | Real failure — needs investigation before review | +| ⏳ | CI running | +| ❓ | No CI result yet (doc-only, draft, or external PR) | +| ⚠️ | Non-blocking failure (e.g. macOS/Windows `continue-on-error`, Codacy ACTION_REQUIRED — maintainer overrides in UI) | + +--- + +## Dependency chains — land in this order + +``` +#121 consolidate/main-hygiene-batch (revert single-tz + sync MEOS API drift; subsumes #116+#118) + └─► #111 fix/per-thread-meos-init (thread-safety foundation) + └─► #58 fix/splitnspans-spanset-result + aggregates batch (12 commits) + └─► #61 fix/geomset-srid-parameter + └─► #106 fix/span-arithmetic-correctness + └─► #107 fix/span-distance-return-types + └─► consolidate/ batch (#97–#105) (parity surface — independent of each other) + └─► #108 feat/parity-math-similarity-tbox + └─► #109 feat/parity-elevation-restrict + └─► #110 feat/parity-split-complement + └─► #112 feat/wkb-roundtrip-all-types (TZ-neutral test fix landed) + └─► #113 feat/edge-to-cloud-quickstart + └─► #114 feat/berlinmod-geo-functions2 + └─► #126 feat/parity-additions-batch (bearing + covers + stbox-dim + seqSetGaps; subsumes #122+#123+#124+#125) + └─► #120 consolidate/pr-coordination-and-tz-lint (subsumes #115+#119) + └─► #117 doc/reviewer-guide (visibility wiring; cross-repo with MobilityDB #931 / MobilitySpark #8) +``` + +**#121 should land first** — it reverts single-timezone state on `main` and syncs MEOS API drift, unblocking every other branch from local-build failures. Then **#111** (per-thread MEOS init) is the thread-safety foundation all parity work builds on. + +The **consolidate/** PRs (#97–#105) are independent parity drops covering different function families; they can land in any order once the four correctness fixes (#58, #61, #106, #107) and #111 are in. + +--- + +## Tier 1 — Merge immediately (bug fixes + visibility, trivially reviewable) + +| PR | Branch | Description | CI | +|----|--------|-------------|----| +| #117 | `doc/reviewer-guide` | **This guide** — PR review ordering, tiers, dependency chains; visibility wiring (PR template + README banner) | ✅ | +| #121 | `consolidate/main-hygiene-batch` | Revert single-tz on main + sync MEOS API drift (subsumes #116+#118); unblocks every other branch | ✅ | +| #120 | `consolidate/pr-coordination-and-tz-lint` | docs/PR-COORDINATION.md + scripts/lint-tz-pinned-tests.py (subsumes #115+#119) | ✅ | +| #111 | `fix/per-thread-meos-init` | Replace global MEOS mutex with per-thread initialization | ✅ | +| #107 | `fix/span-distance-return-types` | Fix distance return types; add `+` / `shift` alias for tstzset/tstzspan+interval | ✅ | +| #106 | `fix/span-arithmetic-correctness` | Fix SpanSet serialization size and floatspan distance datum | ✅ | +| #61 | `fix/geomset-srid-parameter` | `set(LIST(GEOMETRY), INTEGER)` overload — explicit SRID | ✅ | +| #58 | `fix/splitnspans-spanset-result` | `splitNspans` fix on spanset + 10 aggregate-additions (12-commit rolling-topic, scope-creep tolerated) | ✅ | + +--- + +## Tier 2 — Parity surface — consolidate/ batch (independent, all CI green) + +These cover different function families and can land in any order. + +| PR | Branch | Description | CI | +|----|--------|-------------|----| +| #105 | `consolidate/docs` | CONTRIBUTING.md + PARITY.md user guide + PARITY-INVENTORY.md | ✅ | +| #104 | `consolidate/geo-types-parity` | tgeometry + tgeography + tgeogpoint — full parity surface | ✅ | +| #100 | `consolidate/analytics-parity` | Temporal analytics parity — simplify / similarity / tnumber math | ✅ | +| #98 | `consolidate/spatial-predicates-parity` | tspatial predicates parity — topological / comparison / position | ✅ | +| #97 | `consolidate/temporal-ops-parity` | Temporal ops parity — boxops / comparison / position / precision / same | ✅ | +| #103 | `consolidate/aggregates-parity` | Aggregate functions parity — extent / SkipList aggregates / tCentroid | ❌ | +| #102 | `consolidate/tiles-bins-parity` | Tile and bin functions parity — emitters / table functions / getters | ❌ | +| #99 | `consolidate/tgeompoint-ops-parity` | tgeompoint operations parity — distance / affine / transforms / geoMeasure | ❌ | + +--- + +## Tier 3 — Recent feature additions (land after consolidate/ batch) + +| PR | Branch | Description | CI | Notes | +|----|--------|-------------|----|----| +| #126 | `feat/parity-additions-batch` | bearing + eCovers/tCovers + stbox dim + seqSetGaps (subsumes #122+#123+#124+#125) | ✅ | | +| #114 | `feat/berlinmod-geo-functions2` | nearestApproachDistance, expandSpace, `&&` for TGEOMPOINT | ✅ | | +| #113 | `feat/edge-to-cloud-quickstart` | Edge-to-cloud quickstart, temporalFooter(), SRID/geodetic fix, tgeogpoint tests | ✅ | | +| #110 | `feat/parity-split-complement` | timeSplit / valueSplit / quadSplit emitters | ✅ | | +| #109 | `feat/parity-elevation-restrict` | atElevation / minusElevation via public MEOS primitives | ✅ | | +| #108 | `feat/parity-math-similarity-tbox` | Unskip tnumber math, tbox, and similarity parity tests | ✅ | | +| #112 | `feat/wkb-roundtrip-all-types` | Complete binary + hex-WKB round-trip I/O for all types (TZ-neutral test fix landed) | ✅ | | + +--- + +## Review checklist + +For every MobilityDuck PR, verify: + +- [ ] PostgreSQL License header on every new `.cpp` / `.hpp` file +- [ ] New function registered in the correct `RegisterXxx()` function +- [ ] SQL name matches MobilityDB alias (RFC #861 portable SQL contract) +- [ ] NULL input handled (returns NULL or appropriate default) +- [ ] DBL\_MAX sentinel from MEOS mapped to NULL for distance functions +- [ ] New parity test added in `test/` with `nosort` tag where result order is non-deterministic +- [ ] CI green before requesting merge (fix ❌ PRs in-branch, not in a follow-up) From cf36886adb6682a56f3324d709bd1b1340af4be9 Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Wed, 20 May 2026 11:55:01 +0200 Subject: [PATCH 2/5] fix: drop premature `using meosType = MeosType;` alias in tydef.hpp MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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). --- src/include/tydef.hpp | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/src/include/tydef.hpp b/src/include/tydef.hpp index b7b28109..b9860ca0 100644 --- a/src/include/tydef.hpp +++ b/src/include/tydef.hpp @@ -11,11 +11,27 @@ extern "C" { #include } -// Forward-compat alias for the meosType → MeosType rename (MobilityDB -// pr785-sync-script). Vcpkg's MEOS exposes `MeosType`; existing -// MobilityDuck code still uses `meosType`. This alias bridges the two -// without touching every reference site. -using meosType = MeosType; +// MEOS naming history: `meosType` is the **pre-consolidation** spelling +// and `MeosType` is the **post-consolidation** target (the rename is +// part of the upstream consolidation sweep, not yet reached by the +// vcpkg pin). The current pin +// (`vcpkg_ports/meos/portfile.cmake` REF f11b7443ee98…) is still +// pre-consolidation and exposes `meosType` — see +// meos/include/temporal/meos_catalog.h, where line 121 declares +// `} meosType;`. MobilityDuck's source consistently uses +// `meosType` (verified via `grep -rn '\bmeosType\b' src/`), which +// matches the pin, so no alias is needed today. +// +// An earlier version of this file added `using meosType = MeosType;` +// as a forward-looking bridge for the eventual consolidation bump. +// That alias references `MeosType`, which the current pin does NOT +// yet expose, so it broke the build: +// "'MeosType' does not name a type; did you mean 'meosType'?". +// +// When the MEOS pin is bumped past the consolidation point, restore +// a bridge here (`using meosType = MeosType;` becomes valid then) or +// sweep the source `meosType → MeosType` in one PR — whichever the +// project prefers at that time. namespace duckdb { From a17bdf332949583d1596c5fcb1a35505baa72932 Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Fri, 15 May 2026 08:49:29 +0200 Subject: [PATCH 3/5] polyfill(ci): stage icu extension for amd64 docker tests (from open PR #136) Cherry-picked from open PR #136 (commit 9e1d7a6) so this PR's CI goes green before #136 lands. When #136 reaches main, the rebase will collapse this commit to a no-op and it will drop out. --- original commit body --- Pre-stage icu extension for amd64 docker tests LoadInternal calls ExtensionHelper::AutoLoadExtension(db, "icu") so the Europe/Brussels timezone option is honoured. Inside the linux_amd64 test docker container there is no network egress and the local extension directory is empty, so the autoload fails. Copy the icu.duckdb_extension that was just built locally (declared in extension_config.cmake) into the expected path before running the unittester. --- Makefile | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index bc17d050..ab8cf2ee 100644 --- a/Makefile +++ b/Makefile @@ -11,9 +11,32 @@ include extension-ci-tools/makefiles/duckdb_extension.Makefile # both MEOS (meos_initialize_timezone) and DuckDB (DBConfig::SetOptionByName # "TimeZone") to Europe/Brussels. Tests pass on any OS timezone — the # extension is the single source of truth, no TZ env var needed. +# +# LoadInternal also calls ExtensionHelper::AutoLoadExtension(db, "icu") so +# the timezone option is honoured. Autoload looks for the extension on disk +# at $HOME/.duckdb/extensions///icu.duckdb_extension +# and falls back to a hub download. Inside the linux_amd64 test docker +# container that path is empty and there is no network egress, so the +# autoload fails. We copy the icu.duckdb_extension that was built locally +# as part of this extension's build (declared in extension_config.cmake) +# into the expected path before running the unittester. +DUCKDB_VERSION_TAG := v1.4.4 + +define stage_icu + @if [ -f ./build/$(1)/extension/icu/icu.duckdb_extension ]; then \ + platform=$$(uname -m | sed 's/x86_64/linux_amd64/;s/aarch64/linux_arm64/'); \ + target=$$HOME/.duckdb/extensions/$(DUCKDB_VERSION_TAG)/$$platform; \ + mkdir -p "$$target" && cp -f ./build/$(1)/extension/icu/icu.duckdb_extension "$$target/" && \ + echo "Staged icu.duckdb_extension at $$target/"; \ + fi +endef + test_release_internal: + $(call stage_icu,release) ./build/release/$(TEST_PATH) "$(PROJ_DIR)test/*" test_debug_internal: + $(call stage_icu,debug) ./build/debug/$(TEST_PATH) "$(PROJ_DIR)test/*" test_reldebug_internal: - ./build/reldebug/$(TEST_PATH) "$(PROJ_DIR)test/*" \ No newline at end of file + $(call stage_icu,reldebug) + ./build/reldebug/$(TEST_PATH) "$(PROJ_DIR)test/*" From 72e6aa52426fa96dc7e5b2893ee86af6adf9c9d8 Mon Sep 17 00:00:00 2001 From: Esteban Zimanyi Date: Wed, 20 May 2026 19:21:49 +0200 Subject: [PATCH 4/5] polyfill(macos/wasm): bigint_to_set_duckdb int64_t forwarder (from open PR #140) On macOS LP64 and Wasm/emscripten, int64 (long) and int64_t (long long) are the same width but distinct types, so clang rejects passing bigint_to_set where a Set *(*)(int64_t) is expected as a non-type template arg. Cherry- picked from open PR #140 (a8b1755) so this PR goes green on osx_amd64, osx_arm64, and wasm_mvp before #140 lands. The cast is a no-op on Linux, where int64 and int64_t are both long. When #140 reaches main the rebase collapses this commit to a no-op. --- src/temporal/set.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/temporal/set.cpp b/src/temporal/set.cpp index b803498a..1858e489 100644 --- a/src/temporal/set.cpp +++ b/src/temporal/set.cpp @@ -945,6 +945,14 @@ static inline Set *date_to_set_duckdb(DateADT d) { return date_to_set(ToMeosDate(duckdb::date_t(d))); } +// macOS LP64: int64 (long) and int64_t (long long) are the same width but +// distinct types, so clang rejects passing bigint_to_set where a +// Set *(*)(int64_t) is expected as a non-type template arg. The cast is a +// no-op on Linux. See SetUnionScalarFunction below. +static inline Set *bigint_to_set_duckdb(int64_t i) { + return bigint_to_set(static_cast(i)); +} + struct SetPtrState { Set *accumulated; }; @@ -1069,7 +1077,7 @@ void SetTypes::RegisterSetUnionAgg(ExtensionLoader &loader) { LogicalType::INTEGER, SetTypes::intset())); set_union_set.AddFunction( AggregateFunction::UnaryAggregateDestructor>( + SetUnionScalarFunction>( LogicalType::BIGINT, SetTypes::bigintset())); set_union_set.AddFunction( AggregateFunction::UnaryAggregateDestructor Date: Sat, 16 May 2026 11:04:41 +0200 Subject: [PATCH 5/5] Stage icu for the macOS osx_arm64 test path too 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 --- Makefile | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index ab8cf2ee..a9485498 100644 --- a/Makefile +++ b/Makefile @@ -15,16 +15,23 @@ include extension-ci-tools/makefiles/duckdb_extension.Makefile # LoadInternal also calls ExtensionHelper::AutoLoadExtension(db, "icu") so # the timezone option is honoured. Autoload looks for the extension on disk # at $HOME/.duckdb/extensions///icu.duckdb_extension -# and falls back to a hub download. Inside the linux_amd64 test docker -# container that path is empty and there is no network egress, so the -# autoload fails. We copy the icu.duckdb_extension that was built locally -# as part of this extension's build (declared in extension_config.cmake) -# into the expected path before running the unittester. +# and falls back to a hub download. That fails both inside the linux_amd64 +# test docker container (empty path, no network egress) and on the macOS +# osx_arm64 test runner (hub icu not reliably resolvable). We copy the +# icu.duckdb_extension that was built locally as part of this extension's +# build (declared in extension_config.cmake) into the expected path, +# matched to the DuckDB platform string, before running the unittester. DUCKDB_VERSION_TAG := v1.4.4 define stage_icu @if [ -f ./build/$(1)/extension/icu/icu.duckdb_extension ]; then \ - platform=$$(uname -m | sed 's/x86_64/linux_amd64/;s/aarch64/linux_arm64/'); \ + case "$$(uname -s)-$$(uname -m)" in \ + Linux-x86_64) platform=linux_amd64 ;; \ + Linux-aarch64) platform=linux_arm64 ;; \ + Darwin-arm64) platform=osx_arm64 ;; \ + Darwin-x86_64) platform=osx_amd64 ;; \ + *) platform=$$(uname -m) ;; \ + esac; \ target=$$HOME/.duckdb/extensions/$(DUCKDB_VERSION_TAG)/$$platform; \ mkdir -p "$$target" && cp -f ./build/$(1)/extension/icu/icu.duckdb_extension "$$target/" && \ echo "Staged icu.duckdb_extension at $$target/"; \