Skip to content

feat(bindings): splitNspans / splitEachNspans lowercase aliases#8

Merged
nhungoc1508 merged 2 commits into
fix/meos-error-handlerfrom
feat/splitnspans-lowercase-alias
Apr 27, 2026
Merged

feat(bindings): splitNspans / splitEachNspans lowercase aliases#8
nhungoc1508 merged 2 commits into
fix/meos-error-handlerfrom
feat/splitnspans-lowercase-alias

Conversation

@estebanzimanyi

@estebanzimanyi estebanzimanyi commented Apr 24, 2026

Copy link
Copy Markdown
Member

Summary

MobilityDB's SQL surface exposes splitNspans and splitEachNspans (lowercase "spans"). MobilityDuck registered only the camelCase forms splitNSpans / splitEachNSpans in src/temporal/span.cpp, so every parity query using the MobilityDB-native name errored out with "function not found". This PR adds the lowercase aliases for all five set types (intset/bigintset/floatset/dateset/tstzset), pointing at the same underlying SpanFunctions::Set_split_n_spans / SpanFunctions::Set_split_each_n_spans dispatchers. The camelCase forms stay registered for back-compat with existing MobilityDuck callers.

Parity file

Unwraps the splitN/splitEach skip block — 15 queries now active, including the 2 statement error -1 cases that work thanks to the MEOS error handler installed in the parent PR this branch stacks on.

Test plan

  • TZ=UTC ./build/release/test/unittest "<proj>/test/*" locally: 747 assertions pass across 13 test cases, no regressions.
  • CI validates on linux_amd64, linux_arm64, macos, wasm.

Dependencies

Stacked on #7 (fix: install MEOS error handler). This PR's base branch is fix/meos-error-handler, not main — the 2 statement error cases inside the unwrapped block need the error handler from #7 to not kill the test runner. Merge order: #7 → this.

🤖 Generated with Claude Code

@estebanzimanyi estebanzimanyi mentioned this pull request Apr 24, 2026
Merged
2 tasks
@estebanzimanyi @claude @nhungoc1508
fix: install MEOS error handler so errors throw instead of exit()
32151d6 
MEOS's default error handler writes the message to stderr and calls
exit(EXIT_FAILURE) on errlevel == ERROR (PG elog level 21 carried
through via postgres.h). That tears down the whole DuckDB process on
any invalid user input — a parse error on a malformed tstzset literal,
a negative precision on asText, etc. — which (a) is user-hostile in
production and (b) makes every `statement error` test kill the test
runner before Catch2 can check it. PR #6 surfaced this by trying to
exercise MEOS error paths in the parity harness and crashing CI on the
first parse-error query.

This PR installs `MobilityduckMeosErrorHandler` via
`meos_initialize_error_handler(...)` in LoadInternal, right after
`meos_initialize()`. The handler throws a DuckDB::InvalidInputException
on errlevel >= 21 (ERROR/FATAL/PANIC), so MEOS errors surface as
ordinary query failures with the MEOS message attached. Lower levels
(WARNING/NOTICE/INFO) are silently ignored for now — can be wired to
DuckDB's logger in a follow-up if useful.

Parity file (`test/sql/parity/001_set.test`) consequences:

- Deletes the top-of-file error-handler gap note (now obsolete).
- Unwraps the 3 MEOS-error-specific `mode skip` blocks covering tstzset
  parse errors and asText negative-digits — they run as real
  `statement error` assertions.
- Re-wraps ONE of those (`set('{}'::timestamptz[])`) in a skip block
  with a new gap note: that query fails at the DuckDB cast layer with
  a conversion error before MEOS sees it, because `'{}'` is PG
  array-literal syntax that DuckDB rejects as a string literal. The
  equivalent DuckDB form `set(ARRAY[]::TIMESTAMPTZ[])` reaches MEOS
  but the constructor wrapper in src/temporal/set_functions.cpp
  returns null instead of calling meos_error(...) on empty input —
  a separate gap, documented for a later follow-up.

Verified locally (`TZ=UTC ./build/release/test/unittest
"/path/to/test/*"`): 747 assertions pass across 13 test cases (up from
739 pre-handler), no regressions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 25, 2026
Merged
Merged
@estebanzimanyi
feat(bindings): splitNspans / splitEachNspans lowercase aliases
cdea92e 
MobilityDB's SQL surface exposes splitNspans and splitEachNspans
(lowercase "spans"). MobilityDuck registered only the camelCase
forms splitNSpans / splitEachNSpans in src/temporal/span.cpp, so
queries using the MobilityDB-native name errored with "function
not found". Adds the lowercase aliases for all five set types
(intset/bigintset/floatset/dateset/tstzset), pointing at the same
SpanFunctions::Set_split_n_spans /
SpanFunctions::Set_split_each_n_spans dispatchers. The camelCase
forms stay registered for back-compat.

Activates the splitNspans / splitEachNspans queries in
test/sql/parity/001_set.test.
@estebanzimanyi estebanzimanyi force-pushed the feat/splitnspans-lowercase-alias branch from 52feed7 to cdea92e Compare April 25, 2026 08:43
This was referenced Apr 25, 2026
Merged
Merged
Merged
@nhungoc1508 nhungoc1508 merged commit 0c3e04c into fix/meos-error-handler Apr 27, 2026
17 checks passed
@estebanzimanyi estebanzimanyi mentioned this pull request Apr 27, 2026
Merged
@estebanzimanyi

estebanzimanyi commented Apr 27, 2026

Copy link
Copy Markdown
Member Author

@nhungoc1508 — flagging a merge-target issue. This PR is marked MERGED, but the merge commit went into the stack-base branch, not main:

Net effect: this PR's contents never reached main. Verified by greping src/ on origin/main — splitnspans (lowercase) and spans(<set_type>) constructor are absent.

Re-landed in #44, which brings both commits forward as a clean linear history on top of main using the existing branch.

For future stacked PRs: when a base PR (e.g. #7) is merged into main, the dependent PRs (#8, #9) should have their base updated to main (gh pr edit <N> --base main) before merging — otherwise the merge target stays the now-deleted/abandoned base branch and the work doesn't propagate.

@estebanzimanyi estebanzimanyi mentioned this pull request Apr 27, 2026
Merged
estebanzimanyi added a commit that referenced this pull request May 10, 2026
@estebanzimanyi
doc: PR Reviewer Guide + visibility wiring + ecosystem consolidation …
4256154 
…policy

- 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.
estebanzimanyi added a commit that referenced this pull request May 10, 2026
@estebanzimanyi
doc: PR Reviewer Guide + visibility wiring + ecosystem consolidation …
770bfef 
…policy

- 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.
estebanzimanyi added a commit that referenced this pull request May 26, 2026
@estebanzimanyi
doc: PR Reviewer Guide + visibility wiring + ecosystem consolidation …
b5ee4ed 
…policy

- 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@estebanzimanyi @nhungoc1508