Skip to content

test(tgeogpoint): pin TZ display fixtures to Brussels (unblocks #179/#180 linux_amd64)#181

Open
estebanzimanyi wants to merge 8 commits into
MobilityDB:mainfrom
estebanzimanyi:fix/tgeogpoint-test-tz-pinned
Open

test(tgeogpoint): pin TZ display fixtures to Brussels (unblocks #179/#180 linux_amd64)#181
estebanzimanyi wants to merge 8 commits into
MobilityDB:mainfrom
estebanzimanyi:fix/tgeogpoint-test-tz-pinned

Conversation

@estebanzimanyi
Copy link
Copy Markdown
Member

Summary

Stacks on #158 (the substrate that introduced test/sql/tgeogpoint.test). Fixes the four assertions in tgeogpoint.test that hardcode +00 (UTC) instead of +01 (Brussels), so they fail on every CI runner.

Why this fails on CI

MobilityDuck initializes MEOS with meos_initialize_timezone("Europe/Brussels") at extension load. The four affected assertions construct via to_timestamp(unix_seconds) (which is UTC) and then display through MEOS's TZ-aware formatter — which renders as Brussels (UTC+1 in winter, no DST in January).

CI fail message ($179's linux_amd64):

1. /duckdb_build_dir/test/sql/tgeogpoint.test:7
Wrong result in query!
SELECT asText(TGEOGPOINT(ST_Point(1, 2), to_timestamp(946684800)));
Expected: POINT(1 2)@2000-01-01 00:00:00+00
Actual:   POINT(1 2)@2000-01-01 01:00:00+01
test cases: 60 | 59 passed | 1 failed

59/60 passing, 1 failing on this single TZ issue.

The fix

Update four assertions to expect the Brussels TZ output the extension actually produces:

Line Old New
10 POINT(1 2)@2000-01-01 00:00:00+00 POINT(1 2)@2000-01-01 01:00:00+01
15 SRID=4326;POINT(1 2)@2000-01-01 00:00:00+00 SRID=4326;POINT(1 2)@2000-01-01 01:00:00+01
23 [POINT(0 0)@2000-01-01 00:00:00+00, POINT(0 1)@2000-01-02 00:00:00+00] [POINT(0 0)@2000-01-01 01:00:00+01, POINT(0 1)@2000-01-02 01:00:00+01]
28 POINT(1 2)@2000-01-01 00:00:00+00 POINT(1 2)@2000-01-01 00:00:00+01

The first three use to_timestamp(unix_seconds) which parses UTC → displays Brussels (01:00:00+01). The fourth (line 28) uses the EWKT literal 'SRID=4326;Point(1 2)@2000-01-01' which the parser interprets as a Brussels-TZ local datetime, so it stays at 00:00:00+01 (same wall-clock, just with offset annotation).

A header comment now documents the Brussels TZ assumption + cross-references docs/testing-tz-neutral-policy.md.

Branch base

feat/edge-to-cloud-quickstart-rebased (= #158's branch). Stacks PRs #179 and #180 inherit this fix transparently once their bases are bumped (or via rebase).

Local verification

$ ./build/release/test/unittest "test/sql/tgeogpoint.test"
# First 6 assertions pass; subsequent tests are pre-existing #158
# substrate state (env-specific eIntersects on bare-Ubuntu; CI #179 reports
# 1330/1331 — all tests but the TZ one — so on CI this fix lights up
# the full test suite green).

Refs

…eogpoint tests

Adds the user-facing entry point for the edge-to-cloud pipeline plus a
documentation/testing surface around it:

- examples/quickstart/quickstart.sql — 5-vessel synthetic ingest →
  Parquet → Spark-readable layer.
- examples/generic-ingest/generic_ingest.sql — generic CSV → tgeompoint
  ingest template.
- temporalFooter() — builds the TemporalParquet KV_METADATA JSON to
  embed at write time (tgeompoint encoding manifest, schema version,
  timestamp range, SRID).
- SRID/geodetic mixin fix in tgeompoint_functions.cpp — preserves the
  source SRID through cast/transform paths and forces geodetic when
  the input is geographic.
- docs/beta-testing-edge-to-cloud.md — beta-testing guide.
- docs/testing-tz-neutral-policy.md — ecosystem-wide timezone-neutral
  test policy (canonical reference for MobilityDB / MobilityDuck /
  PyMEOS / JMEOS / meos-rs).
- docs/tgeogpoint-design.md — design notes.
- test/sql/tgeogpoint.test, test/sql/parquet/temporal_parquet.test —
  test coverage for the new pieces.
- .gitignore + CMakeLists.txt + extension load — wiring.
`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).
…MobilityDB#136)

Cherry-picked from open PR MobilityDB#136 (commit 9e1d7a6) so this PR's CI goes
green before MobilityDB#136 lands. When MobilityDB#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.
…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.
…ion init)

MobilityDuck initializes MEOS with `meos_initialize_timezone("Europe/Brussels")`
in the extension entry point.  tgeogpoint.test had four assertions hardcoded
to `+00` (UTC) instead of `+01` (Brussels winter), so they failed on every CI
runner.

The four affected assertions are all winter-date constructors that go
through `to_timestamp(unix_seconds)` — those parse as UTC then display in
the extension's runtime TZ (Brussels = UTC+1 in January, no DST).  Updates:

  - asText(TGEOGPOINT(...))              -> POINT(...)@2000-01-01 01:00:00+01
  - asEWKT(TGEOGPOINT(...))              -> SRID=4326;POINT(...)@... 01:00:00+01
  - asText(tgeogpointSeq(ARRAY[..., ...])) -> [POINT(...)@... 01:00:00+01, ...]

A fifth literal-constructor case (`tgeogpoint 'SRID=4326;Point(1 2)@2000-01-01'`)
uses the EWKT parser which interprets the literal date as a Brussels-TZ
local time, so it stays at `00:00:00+01` (not 01:00:00+01).

The header comment now documents the Brussels TZ assumption + cross-refs
docs/testing-tz-neutral-policy.md so future test authors don't repeat
this.
@estebanzimanyi
Copy link
Copy Markdown
Member Author

Substrate diagnostic — eIntersects(GEOMETRY, TGEOGPOINT) line-56 fail (the 1 remaining red after this PR's TZ fix)

Investigated locally; partial findings + remaining gap below for the next investigator.

Two stale-but-still-loaded registrations identified:

src/geo/tgeogpoint.cpp registers {GeoTypes::GEOMETRY(), TGEOGPOINT()} -> TgeompointFunctions::Eintersects_geo_tgeo (note: TgeoMpointFunctions, not TgeoGpointFunctions) at line 1320–1326. The underlying impl in src/geo/tgeompoint_functions.cpp:1853 hardcodes int32 srid = 0; before deserialization, which collides with the TGEOGPOINT's SRID=4326. Same pattern for eDisjoint (1256), aDisjoint (1283), aIntersects (1346).

Tried a fix and it doesn't reach:

A new GeoTgeoIntExec_FromTgeoGeo template in src/geo/tgeogpoint_ops.cpp (mirrors the existing GeoTgeoDistIntExec_FromTgeoGeo for dwithin) wired into EA_REG_2_COMMUT for the (GEOM, TGEOM) direction; and the 4 stale tgeogpoint.cpp registrations removed. The new executor compiles + lands in the binary (verified via nm) but does not fire at runtime even when probed with a unique throw. So a third registration path is shadowing the macro path; haven't located it.

Local repro on a host with the patch applied:

LOAD mobilityduck; LOAD spatial;
SELECT eIntersects(
  ST_GeomFromText('POLYGON((0 0, 2 0, 2 2, 0 2, 0 0))'),
  TGEOGPOINT(ST_Point(1, 1), to_timestamp(946684800))
);
-- Still: Invalid Input Error: The value must be a spatiotemporal value

vs. the working (TGEOGPOINT, GEOM) direction which gets past the validation and surfaces a different, real MEOS-side error:

-- Invalid Input Error: Operation on mixed planar and geodetic coordinates

That second error (mixed planar/geodetic) is the actual semantic bug the eIntersects fix needs to address — the bbox SRID/geodetic-flag plumbing inside MEOS — and is consistent with what PR #158's commit comment described as "Bug 2: FLAGS_SET_GEODETIC alone corrupts bbox layout".

Next step: locate the third {GEOMETRY, TGEOGPOINT} registration that's winning over both tgeogpoint.cpp's and tgeogpoint_ops.cpp's, then apply the SRID-from-tspatial_srid + geom_to_geog fix in the actually-invoked executor.

This PR's TZ fix is independently correct and CI-validated (1330/1331 → 1335/1336 = +5 assertions); the remaining red is exactly this line-56 substrate bug, not introduced by this PR.

@estebanzimanyi
Copy link
Copy Markdown
Member Author

Substrate finding for the amd64 line-56 eIntersects(GEOMETRY, TGEOGPOINT) failure

Status: root cause located; one-line fix not possible. Needs design input.

Symptom

SELECT eIntersects(ST_GeomFromText('POINT(1 1)')::GEOMETRY,
                   '[Point(1 1)@2000-01-01, Point(2 2)@2000-01-02]'::TGEOGPOINT);
-- Invalid Input Error: The value must be a spatiotemporal value

Root cause

DuckDB function resolution treats GEOMETRY, TGEOMPOINT, TGEOGPOINT, TGEOMETRY, TGEOGRAPHY as alias-equivalent because every one of them is a LogicalType::BLOB with an alias label. For eIntersects(GEOMETRY, TGEOGPOINT), every two-arg eIntersects overload (whatever its declared SQL types) is scored equally — earlier-registered wins.

TgeompointType::RegisterScalarFunctions (mobilityduck_extension.cpp:293) registers eIntersects-family overloads earlier than TGeogpointOps::RegisterScalarFunctions (line 318), so the call routes to TgeompointFunctions::Eintersects_tgeo_geo (src/geo/tgeompoint_functions.cpp:1889), declared for {TGEOMPOINT, GEOMETRY}.

That executor blindly reinterpret_cast<Temporal*>(args.data[0].GetData()) and calls tspatial_srid(tgeom) on it. When args.data[0] is actually a GEOMETRY blob, ensure_tspatial_type fails inside MEOS — that's where the error string comes from.

What the WIP commits already do (kept, doesn't fully resolve the issue)

  • Removed 4 stale direct registrations of (GEOMETRY, TGEOGPOINT) in src/geo/tgeogpoint.cpp (eDisjoint/aDisjoint/eIntersects/aIntersects) that routed to the SRID-0-hardcoded TgeompointFunctions::E*_geo_tgeo family — wrong for geodetic input.
  • Added a new GeoTgeoIntExec_FromTgeoGeo template + extended EA_REG_2_COMMUT in tgeogpoint_ops.cpp / tgeography_ops.cpp / tgeometry_ops.cpp to register the (GEOMETRY, TGEO*) direction with arg-order swapped at the MEOS call site.

These remain correct: when DuckDB does pick the macro's overload (e.g. for cleanly disambiguated calls), it produces geodetically-correct results. They just can't override an earlier-registered alias-equivalent overload.

Empirical confirmation

Three labelled throw InvalidInputException debug markers in the new GeoTgeoIntExec_FromTgeoGeo template (one per TU: TGEOGPOINT/TGEOGRAPHY/TGEOMETRY) — none fire for eIntersects(GEOMETRY, TGEOGPOINT), even when the macro is reduced to register only the {GEOM, TGEOM} line. The call always reaches tspatial_srid via the earlier-registered Eintersects_tgeo_geo.

Two viable fix shapes

(A) Defensive arg-typing in every shared executor. Detect whether args.data[0] is a Temporal or a GSERIALIZED before dispatch (e.g. by inspecting the leading bytes for a Temporal subtype tag), and swap arg order if needed. Touches every spatial-rel executor that takes BLOB-aliased types in either order; mechanical but invasive (~20-30 executors).

(B) Make the alias chain structurally distinguishable. Switch GEOMETRY/TGEO* LogicalTypes from BLOB+alias to distinct internal structures (e.g. LogicalType::STRUCT with one BLOB field, different LogicalTypeIds) so DuckDB's resolution stops treating them as equivalent. Deeper redesign; affects every type registration.

(A) is the lower-risk, higher-test-coverage path; (B) is the principled long-term fix.

Until either fix lands

The TZ fix in PR #181 still moves CI from 1330/13311335/1336. Line 56 (this eIntersects case) is the remaining red on amd64; arm64 also exhibits it via the same code path. Other PRs that inherit the same substrate via the open-PR base (#158): #179, #180, #182, #183 — same residual line-56 fail until (A) or (B) lands.

…spatch

DuckDB function resolution treats GEOMETRY, TGEOMPOINT, TGEOGPOINT,
TGEOMETRY, TGEOGRAPHY as alias-equivalent because each is a
LogicalType::BLOB with an alias label. The constant-folder routes
eIntersects(GEOMETRY, TGEOGPOINT) into the {TGEOMPOINT, GEOMETRY}
overload of TgeoGeoIntExec from src/geo/tgeometry_ops.cpp (earlier
registered), which assumes args.data[0] is the Temporal and feeds
the GEOMETRY blob to tspatial_srid — hitting ensure_tspatial_type
inside MEOS with 'must be a spatiotemporal value' before any of the
correctly-direction-aware *_geo_tgeo executors get a chance to run.

Add BlobLooksLikeTemporal to geo_util.hpp: a pure-predicate probe
that reads byte 4 of the blob (Temporal's temptype field) and runs
it through tspatial_type — true only for T_TGEOMPOINT / T_TGEOGPOINT
/ T_TGEOMETRY / T_TGEOGRAPHY / T_TRGEOMETRY. DuckDB GEOMETRY blobs
have byte 4 in their WKB header which never matches one of those
MeosType enum values.

Use the probe at the top of TgeoGeoIntExec in all three *_ops.cpp
TUs to detect mis-ordered args and silently swap roles before
decoding. Also extend each executor with the geodetic-aware
geom_to_geog conversion (matching the existing pattern in
TgeompointFunctions::Eintersects_*) so the swapped-direction call
still respects MEOS_FLAGS_GET_GEODETIC.

Also fixes a unique_ptr<BinsBindData> -> unique_ptr<FunctionData>
build break in span_table_functions.cpp's BinsBindData::Copy() that
otherwise blocks the substrate fix landing alongside the test.

Locally green: test/sql/* full suite, 60/60 test cases, 1346/1346
assertions (was 1335/1336 with the failing line-56 eIntersects).
gdb-confirmed: BlobLooksLikeTemporal probe returns a_is_temp=0
b_is_temp=1 for the (GEOMETRY, TGEOGPOINT) case, swap routes
TGEOGPOINT to the Temporal path.
@estebanzimanyi
Copy link
Copy Markdown
Member Author

Substrate fix landed in commit 7d3856e

Building on the diagnosis in the previous comment, the gdb backtrace pinpointed the active executor as (anonymous namespace)::TgeoGeoIntExec<&eintersects_tgeo_geo> from src/geo/tgeometry_ops.cpp's anonymous-namespace instantiation (earliest-registered {GEOM, TGEO*} overload — DuckDB's constant-folder picked it for the eIntersects(GEOMETRY, TGEOGPOINT) call). The lambda body assumed args.data[0] is the Temporal and fed the GEOMETRY blob straight to tspatial_srid, which raised "must be a spatiotemporal value" from MEOS.

Fix: introduce BlobLooksLikeTemporal(string_t) in src/include/geo_util.hpp — a pure-predicate probe reading the blob's byte 4 (Temporal temptype field per the { int32 vl_len_; uint8 temptype; uint8 subtype; int16 flags; ... } layout) and running it through tspatial_type. Used at the top of TgeoGeoIntExec in all three *_ops.cpp TUs to detect mis-ordered args and swap roles silently. Also extends each executor with the geodetic-aware geom_to_geog conversion (matching the existing pattern in TgeompointFunctions::Eintersects_*) so the now-correctly-routed call respects MEOS_FLAGS_GET_GEODETIC.

Gdb-confirmed via BlobLooksLikeTemporal probe: a_is_temp=0 a_byte4=0 a_sz=120 (GEOMETRY blob) / b_is_temp=1 b_byte4=47 b_sz=48 (TGEOGPOINT — T_TGEOGPOINT=47). Swap routes TGEOGPOINT to the Temporal path, GEOMETRY to the GSerialized path; geodetic-flag conversion then handles the lon/lat input correctly.

Local CI mirror: make test — 60/60 test cases, 1346/1346 assertions pass (was 1335/1336 with the failing line-56). Also fixes an unrelated unique_ptr<BinsBindData> → unique_ptr<FunctionData> build break in span_table_functions.cpp that was blocking compilation.

Pushing — CI on the rebased branch should now go green on Linux amd64.

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