Skip to content

cleanup(geo): retire stale Spherical_lonlat workaround; re-enable GEODSTBOX ZT area test#165

Open
estebanzimanyi wants to merge 5 commits into
MobilityDB:mainfrom
estebanzimanyi:cleanup/retire-geodetic-stbox-workarounds
Open

cleanup(geo): retire stale Spherical_lonlat workaround; re-enable GEODSTBOX ZT area test#165
estebanzimanyi wants to merge 5 commits into
MobilityDB:mainfrom
estebanzimanyi:cleanup/retire-geodetic-stbox-workarounds

Conversation

@estebanzimanyi
Copy link
Copy Markdown
Member

@estebanzimanyi estebanzimanyi commented May 20, 2026

Summary

src/geo/stbox_functions.cpp carries three helpers — Spherical_lonlat_rect_area_m2, Geodetic_stbox_footprint_area, Stbox_geodetic_xy_copy — that workaround MEOS-1.3 bugs that the pinned MEOS-1.4 integration tip does not have:

Helper MEOS-1.4 status
Spherical_lonlat_rect_area_m2 / Geodetic_stbox_footprint_area stbox_area(GEODSTBOX ZT(((1,2,3),(4,5,6)),[2000-01-01,2000-01-02])) returns 110593375170.3 cleanly — no SIGSEGV on the PolyhedralSurface path
Stbox_geodetic_xy_copy stbox_to_geo projects 3D geodetic boxes to a 2D POLYGON with Z=0 internally
(geog_in WKT fall-through) fixed upstream in MobilityDB PR #1090 (closes #1089)

This PR

  • removes the three workaround helpers
  • replaces Stbox_area with a direct stbox_area call (the dead == DBL_MAX error sentinel becomes < 0, matching MEOS's documented -1.0 on error)
  • simplifies Stbox_expand_space to call stbox_to_geo directly
  • re-enables the GEODSTBOX ZT area test that was commented out, with MEOS's value (110593375170.3) as the expected output
  • adds GEODSTBOX X and GEODSTBOX Z area tests for coverage

Verification

Direct probes against the pinned MEOS-1.4 (integration/meos-1.4-bump):

GEODSTBOX ZT(((1.0,2.0,3.0),(4.0,5.0,6.0)),[2000-01-01,2000-01-02])
    stbox_area(spheroid=true)  = 110593375170.3
    stbox_area(spheroid=false) = 111083945187.3
GEODSTBOX X((1.0,2.0),(3.0,4.0))
    stbox_area(spheroid=true)  = 49173168695.2
GEODSTBOX Z((1.0,2.0,3.0),(4.0,5.0,6.0))
    stbox_area(spheroid=true)  = 110593375170.3

No SIGSEGV on any path.

Dependency

Requires the MEOS-1.4 vcpkg pin from #164 (or any pin that includes #1090). This PR stacks on #164.

Files changed

src/geo/stbox_functions.cpp | 51 ++++++-----------------------------------
test/sql/stbox.test         | 19 +++++++++++++-----
2 files changed, 18 insertions(+), 51 deletions(-)

Switches `vcpkg_ports/meos/portfile.cmake` REF from the pre-bump
`f11b7443e` (MobilityDB#768) to the user-fork integration branch
`estebanzimanyi/MobilityDB:integration/meos-1.4-bump` at commit
`80c24f46d`, which merges all 15 open MEOS-1.4 bump PRs into one
buildable commit:

* #1075 portable bare-name aliases     * #1090 geog_in fall-through fix
* #1081 tcbuffer accessors             * #1094 tolerance fix (0.0)
* #1082 tnpoint accessors              * #1095 meos_error contract doc
* #1083 tcbuffer/tpose ctors           * #1096 meos_error CI ratchet
* #1084 from_base time ctors           * #1097 per-site fall-through fixes
* #1085 tpose_from_mfjson              * #1098 math/aggfuncs/datagen fixes
* #1088 geodetic spatialrels           * #1100 ea_* geodetic completion
                                        * #1101 signature uniformization

Both libmeos and libMobilityDB-1.4 built green from this SHA
(Linux x86_64).  Switches the pin **back to MobilityDB/MobilityDB**
once the 15 PRs land on master — that's a one-line edit (REPO
+ REF + SHA512); no other code change.

Why this is the right move now: the bump's source-of-truth IS the
15 open green PRs — pinning to their integration SHA lets every
downstream consumer pick up the new MEOS surface today, without
waiting on the review-merge cascade.  Unblocks specifically:

* The `*_port_core` stack (MobilityDB#148 / MobilityDB#150 / MobilityDB#151 / MobilityDB#153 / MobilityDB#155 / MobilityDB#156)
  which needs the `tcbuffer*` / `tnpoint*` / `tpose*` symbols
  exported by #1081–#1085 (currently 0/0/0 in the pre-bump pin).
* The TRTREE / MEST work (MobilityDB#143 / MobilityDB#144) which composes with the
  uniformized array-return signatures from #1101.
* The geodetic correctness story (the 3 preview releases) which
  consumes #1088 / #1090 / #1094 / #1100 ea_* completion.

Also updates the runtime version stamp `MOBILITYDUCK_MEOS_PIN` in
`src/mobilityduck_extension.cpp` so `SELECT mobilityduck_full_version()`
reports the new pin.

Refs: MobilityDB #1075, #1081-1085, #1088, #1090, #1094, #1095-1098,
#1100, #1101 (the 15 open bump PRs).
…tbox_area / stbox_to_geo

The Spherical_lonlat_rect_area_m2 / Geodetic_stbox_footprint_area /
Stbox_geodetic_xy_copy helpers were workarounds for two MEOS bugs that
no longer exist on the pinned MEOS-1.4 integration tip:

  - geog_in WKT fall-through SIGSEGV — FIXED upstream in MobilityDB
    PR #1090 (closes #1089).
  - stbox_area / stbox_to_geo SIGSEGV on 3D geodetic boxes — verified
    NOT-a-current-bug across multiple direct probes:
      stbox_area(GEODSTBOX ZT(((1,2,3),(4,5,6)),[2000-01-01,2000-01-02]))
      returns 110593375170.3 cleanly on the integration MEOS.

Replace the two-branch workaround in Stbox_area with a direct
stbox_area call, drop the manual Z-strip in Stbox_expand_space (MEOS
stbox_to_geo already projects 3D geodetic boxes to a 2D POLYGON with
Z=0 internally), and re-enable the previously commented test for the
GEODSTBOX ZT area case plus two new tests for X and Z geodetic shapes.

Also fix the existing error-sentinel check on Stbox_area: MEOS
stbox_area returns -1.0 on error (null box or no X dimension), not
DBL_MAX (an artifact of the spherical-approximation workaround path).

Stacks on PR MobilityDB#164 (vcpkg pin bump to MEOS-1.4-integration).
…en PR MobilityDB#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 MobilityDB#140 (a8b1755) so this PR goes green on osx_amd64,
osx_arm64, and wasm_mvp before MobilityDB#140 lands. The cast is a no-op on Linux,
where int64 and int64_t are both long. When MobilityDB#140 reaches main the rebase
collapses this commit to a no-op.
estebanzimanyi and others added 2 commits May 21, 2026 15:50
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>
`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: MobilityDB#126, MobilityDB#130, MobilityDB#149,
MobilityDB#158, MobilityDB#159, MobilityDB#160, plus the entire `feat/*_port_core` extended-type
stack (MobilityDB#148/MobilityDB#150/MobilityDB#151/MobilityDB#153/MobilityDB#155/MobilityDB#156).
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