Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<!--
👀 Reviewers: this project's tier ranking, dependency chains and the
standards checklist live in:
doc/contributing/reviewer-guide.md
PR authors: update that file in the same commit as any PR queue change.
-->

## Summary

<!-- What this PR does, in 1–3 bullets. Focus on the "why". -->

## Test plan

<!-- - [ ] Bullet checklist of TODOs for verifying this PR -->
32 changes: 31 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,39 @@ 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/<duckdb_version>/<platform>/icu.duckdb_extension
# 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 \
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/"; \
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/*"
$(call stage_icu,reldebug)
./build/reldebug/$(TEST_PATH) "$(PROJ_DIR)test/*"
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
119 changes: 119 additions & 0 deletions doc/contributing/reviewer-guide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
<!--
Copyright(c) MobilityDB Contributors

This documentation is licensed under a
Creative Commons Attribution-Share Alike 3.0 License
https://creativecommons.org/licenses/by-sa/3.0/
-->

# 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)
26 changes: 21 additions & 5 deletions src/include/tydef.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,27 @@ extern "C" {
#include <meos_internal.h>
}

// 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 {

Expand Down
10 changes: 9 additions & 1 deletion src/temporal/set.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<int64_t, ...> below.
static inline Set *bigint_to_set_duckdb(int64_t i) {
return bigint_to_set(static_cast<int64>(i));
}

struct SetPtrState {
Set *accumulated;
};
Expand Down Expand Up @@ -1069,7 +1077,7 @@ void SetTypes::RegisterSetUnionAgg(ExtensionLoader &loader) {
LogicalType::INTEGER, SetTypes::intset()));
set_union_set.AddFunction(
AggregateFunction::UnaryAggregateDestructor<SetPtrState, int64_t, string_t,
SetUnionScalarFunction<int64_t, bigint_to_set>>(
SetUnionScalarFunction<int64_t, bigint_to_set_duckdb>>(
LogicalType::BIGINT, SetTypes::bigintset()));
set_union_set.AddFunction(
AggregateFunction::UnaryAggregateDestructor<SetPtrState, double, string_t,
Expand Down
Loading