Skip to content

feat(parity): th3index — full H3 cell index API (66 MEOS exports) [supersedes #129]#178

Open
estebanzimanyi wants to merge 3 commits into
MobilityDB:feat/parity-final-batchfrom
estebanzimanyi:feat/parity-th3index
Open

feat(parity): th3index — full H3 cell index API (66 MEOS exports) [supersedes #129]#178
estebanzimanyi wants to merge 3 commits into
MobilityDB:feat/parity-final-batchfrom
estebanzimanyi:feat/parity-th3index

Conversation

@estebanzimanyi
Copy link
Copy Markdown
Member

Summary

Supersedes #129, which lives on the org branch MobilityDB/MobilityDuck:feat/parity-th3index and predates the fork-branch-only PR policy. The org branch is conflict-stuck (rebased from the old feat/parity-final-batch tip).

This PR ships the identical th3index payload (66-MEOS-export H3 cell index API) cherry-picked cleanly onto the current feat/parity-final-batch tip, plus the one polyfill needed for the rebased base to build under the DuckDB 1.4.4 ABI.

Commit Purpose
feat(parity): th3index — full H3 cell index API (66 MEOS exports) The 66-export H3 surface (Bytea-WKB, JSON, set ops, indexing). 711-LoC implementation + 118-LoC header.
polyfill(span_table_functions): apply PR #173 unique_ptr fix DuckDB 1.4.4 disallows implicit derived->base unique_ptr conversion. Mirrors PR #173 verbatim.

File-by-file

Path Change
src/h3/th3index.cpp New — 711 LoC, 66 MEOS-export wrappers
src/include/h3/th3index.hpp New — class surface + UDF declarations
src/mobilityduck_extension.cpp +5 — register the H3 type + UDFs
CMakeLists.txt +26 — wire the new translation unit + h3 dep
vcpkg.json +1 — add h3 dependency
vcpkg_ports/meos/portfile.cmake +136 — pass-through H3 feature flag to MEOS port
vcpkg_ports/meos/vcpkg.json +3 — declare H3 as a feature of the MEOS port
src/temporal/span_table_functions.cpp +3 −1 — polyfill of PR #173

Verification

Built locally green against current feat/parity-final-batch tip (full release target — 195/195 ninja targets linked, test/unittest binary produced).

Action requested

Wraps Uber's H3 discrete global grid as MobilityDuck SQL types:

- `H3INDEX`  — 64-bit cell id (BIGINT alias).
- `TH3INDEX` — Temporal* of H3 cells (BLOB alias); all four
  subtypes (instant / discrete sequence / step sequence / linear
  sequence / sequence set).

## Surface registered (66 MEOS exports)

- Inout: text parse / format, asBinary/asEWKB/asHexWKB/asMFJSON,
  fromBinary / fromHexWKB / fromMFJSON.
- Constructors: H3 cell from lat/lng+res, tinstant/tsequence/
  tsequenceset constructors.
- Accessors: subtype/interp/memSize/numInstants/numSequences/
  startInstant/endInstant/start{Timestamp,Value}/end{Value}/
  values/valueAtTimestamp.
- Casts: th3index → tgeompoint / tgeogpoint (each with res),
  th3index → tbigint (deferred; tbigint not yet exposed).
- H3 metric/topology surface: cell area / edge length / unit
  string dispatch, cell→parent / center-child / child-pos / vertex,
  hierarchy, neighbors, geo→cell sets (static geometry → H3 cell
  set), every*/always* comparison predicates, tEq/tNe/tDistance.

## Build & port plumbing

- Bump MEOS port pin to `feat/th3index-complete` tip (`beddae670`
  upstream) and enable `-DH3=ON`.
- vcpkg.json: add `h3` as a top-level dependency.
- CMakeLists.txt: `find_package(h3 CONFIG)` plus explicit
  `find_path(h3api.h)` probe — vcpkg's h3 imported target carries
  `include/` only; MEOS source uses `#include <h3api.h>` which
  lives at `include/h3/`, so we publish that subdir as an include
  directory.

## Upstream MEOS gaps patched via portfile

The MEOS source at `beddae670` has four standalone-build gaps
visible only with `-DH3=ON`; vcpkg port applies these workarounds
until they land upstream:

1. `meos/include/h3/th3index_internal.h` unconditionally
   `#include <fmgr.h>` (PG-only).  Guarded with `#if !MEOS`,
   mirroring the same idiom in `meos/include/temporal/temporal.h`.
2. `meos/CMakeLists.txt` builds the `h3` OBJECT library but the
   `PROJECT_OBJECTS` list — fed to `add_library(meos ...)` —
   silently omits `h3`.  Without this, libmeos shipped with zero
   H3 symbols even when `H3=ON`.
3. `meos/CMakeLists.txt` install rules carry every other module
   header (`meos_npoint.h` / `meos_pose.h` / `meos_rgeo.h` /
   `meos_cbuffer.h`) but no `install()` for `meos_h3.h`.
4. `meos/src/h3/{h3_geo,th3index_latlng,th3index_metrics}.c` call
   `ensure_srid_is_latlong()` (declared in
   `meos/include/geo/tgeo_spatialfuncs.h`) without including that
   header → implicit-declaration error under `MEOS=1`.

Also: `H3_LIBRARY` and `H3_INCLUDE_DIR` are now passed explicitly
to the MEOS configure (resolving from `${CURRENT_INSTALLED_DIR}/
lib` and `${CURRENT_INSTALLED_DIR}/include/h3`), bypassing MEOS's
own `find_library(NAMES h3)` which does not consult vcpkg's
toolchain on every triplet (notably `arm64-linux-release`).

## Compile-side wrappers

- Local `h3index_in` / `h3index_out` definitions (in `extern "C"`
  in `src/h3/th3index.cpp`) since MEOS declares these in
  `meos_h3.h` but does not provide a standalone-build definition;
  the wrappers route through h3's own `stringToH3` / `h3ToString`.
- `th3index<->tbigint` cast registrations are deferred until
  MobilityDuck exposes the `tbigint` base type (a separate
  follow-up).

Squash of 61 iteration commits on `feat/parity-th3index`; full
history preserved on `backup/parity-th3index-orig`.
PR MobilityDB#129's base (feat/parity-final-batch / PR MobilityDB#130) does not yet carry
the PR MobilityDB#173 fix for src/temporal/span_table_functions.cpp:47.  Cherry-
picking the th3index payload onto current MobilityDB#130 inherits the DuckDB
1.4.4 implicit derived->base unique_ptr conversion error.

This polyfill applies the same fix as MobilityDB#173 (uses unique_ptr_cast),
matching the polyfill pattern from the feat/geography-* stack.
@estebanzimanyi
Copy link
Copy Markdown
Member Author

CI state (initial run)

Job Result Time
Linux x86_64 ❌ SIGSEGV in tgeompoint.test (test 71/73) 25m55s
Linux arm64 ✅ PASS 20m21s

The amd64 SIGSEGV is post-test-startup (after 70 tests passed); locally on the same SHA the test fails only with a timezone-mismatch (+01 vs +00 — known Europe/Brussels vs UTC pinning), not SIGSEGV. arm64 with the same SHA passes the full suite. This looks like an amd64-runner-specific issue or interaction in the test runner's process state, not a payload defect. Investigation continues; payload is sound on arm64 + locally.

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>
@estebanzimanyi
Copy link
Copy Markdown
Member Author

Same SIGSEGV pattern as #134 stack

The Linux amd64 SIGSEGV on this PR (in tgeompoint.test) is the same root cause as the cluster across PRs #134/#145/#148-#156. CI hits:

/duckdb_build_dir/duckdb/test/sqlite/test_sqllogictest.cpp:216: FAILED:
  SIGSEGV - Segmentation violation signal

across different test fixtures (this PR's tgeompoint.test, #134's parity/042_tgeogpoint_parity.test, #149's parity/001_set.test) — meaning the crash isn't fixture-specific but a class of crash that any MEOS-touching test surfaces under the new pin (commit 80c24f46d042baa2613515b0f5b82255f21fb522).

See #134's analysis comment for the bump-range survey (58 commits across 276 files, 83 MEOS source files) and the diagnostic narrowing that confirms the crash is in the MobilityDuck↔MEOS boundary, not pure MEOS. Reproduction recipe + next-touch path documented there.

This PR's separate substantive work (th3index full H3 cell index API, 66 MEOS exports) is unblocked by upstream MobilityDB#1106 + #1107; the SIGSEGV that gates CI is shared infrastructure.

estebanzimanyi added a commit to estebanzimanyi/MobilityDuck that referenced this pull request May 21, 2026
…t_functions.cpp

PR MobilityDB#149's source-rename cherry-picks (2473d38 temptype rename −1087 lines,
99d6711 drop restr/at_value, 4d125ff 2-arg API) were based on an earlier
parity state and clobbered the parity-final-batch / th3index function
definitions (Temporal_time_bins, Tnumber_*_boxes, Acovers/Ecovers/Tcovers
family, Bearing_*, Tspatial_transform_pipeline) that the registration
tables in temporal.cpp / tgeogpoint.cpp / *_ops.cpp still reference —
producing 'undefined reference' link errors.

Restore both files from feat/parity-th3index (the complete superset that
PR MobilityDB#178 carries) and re-apply only the temptype_continuous ->
temptype_supports_linear rename via sed, preserving every parity
definition while keeping the new-MEOS symbol name.
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