Skip to content

feat: minimal engine enablers for the GQL extension (incl. standalone-call rewrite batch fix) - #1104

Merged
adsharma merged 4 commits into
LadybugDB:mainfrom
chiangchenghsin-hash:feat/gql-engine-enablers
Oct 4, 2026
Merged

adsharma merged 4 commits into
LadybugDB:mainfrom
chiangchenghsin-hash:feat/gql-engine-enablers

Conversation

@chiangchenghsin-hash

Copy link
Copy Markdown
Contributor

Per #1096, this is the engine-side half of the GQL contribution: four small, independent concerns (~48 lines total) that the GQL→Cypher translation layer needs from core. Happy to split into separate PRs if preferred.

Changes

  1. fix(parser): reset standalone-call rewrite query per statement (standalone_call_rewriter.cpp, +4) — getRewriteQuery() left the previous statement's rewrite text in place, so a multi-statement batch could leak one statement's rewrite into statements that have none (and into statements whose rewrite failed to fire). Mechanical reset; see the testing note below.
  2. feat(catalog): function lookup falls back from graph catalogs to main (catalog.h/.cpp, database_manager.cpp) — Catalog::setFunctionFallback lets a graph catalog resolve functions that live in the main catalog. Without it, USE GRAPH makes any main-catalog function call fail with "function does not exist".
  3. feat(extension): per-database extension state slots (extension_manager.h/.cpp) — ExtensionManager::setData/getData attach a named per-database blob to an extension. The GQL extension uses the slot for its graph-type registry (per-database, survives across calls).
  4. fix(binder): set alias on bound path expressions (bind_graph_pattern.cpp, +4) — BoundPathExpression never carried its name; whole-path predicates (IS_TRAIL(p) etc.) emitted by the GQL translation layer need to resolve it. Naming only; no query-semantics change.

Types of changes

  • Bug fix
  • New feature

Testing

  • Full engine regression: 1975/1975, 0 failures on Linux (ubuntu-latest, gcc, -Werror) including these changes: CI run 37164343158. On Windows/MSVC: 1972/1975 — the 3 failures are the extension-registry INSTALL cases in a build config where CMake's optional find_package(OpenSSL 3) is not satisfied (bundled cpp-httplib built without SSL); they pass with OpenSSL + network and are unrelated to these changes.
  • No behavior change for previously passing queries — the full suite is green in both environments.
  • On per-concern tests: rewriteFunc has no in-tree provider (the hook is assigned only by extensions), so the leak in (1) is not observable in core tests today; (2)–(4) are consumed by the GQL extension's suite. Positive coverage arrives with the extension PR to LadybugDB/extensions (161 dual-run parity cases + an opengql/tck run — see Feature: ISO GQL compatibility layer extension (CALL GQL → Cypher translation) — contribution offer / RFC #1096). Happy to add a core-level test if there is a way to exercise rewriteFunc in-tree that we have missed.

Checklist

  • I have changed storage version if on disk format has changed. (n/a)
  • I have requested a review from a maintainer.
  • I have updated the documentation (if needed). (n/a — internal APIs)

Chiang Chenghsin added 4 commits October 4, 2026 08:47
getRewriteQuery() left the previous statement's rewrite text in place, so a
multi-statement batch could leak one statement's rewrite into statements
that have none (and into statements whose rewrite failed to fire).

rewriteFunc has no in-tree provider yet (the hook is exercised by the GQL
extension suite in LadybugDB/extensions), so this is not observable in core
tests today; the fix is the mechanical reset.
Catalog::setFunctionFallback lets a graph catalog resolve functions that
live in the main catalog; without it, USE GRAPH makes CALL of any
main-catalog function fail with 'function does not exist'. Required by
the GQL extension, whose functions are registered on main.
ExtensionManager::setData/getData attach a named, per-database blob to an
extension (ownership: the extension). The GQL extension uses the slot for
its graph-type registry, which must survive across calls and be scoped per
database.
BoundPathExpression never carried its name, so consumers that need the
path variable's name (e.g. whole-path predicates like IS_TRAIL(p) emitted
by the GQL translation layer) could not resolve it. Naming only; no
query-semantics change.
@adsharma

adsharma commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

This looks good to go. Some followups/questions:

  • Low: no in-tree regression test for the rewrite fix — a stub-rewriteFunc unit test is feasible; acceptable to defer coverage to the extension PR's parity suite if recorded explicitly
  • Low: dataMap has no locking and flat key namespace — matches file's existing posture, but document the single-threaded-init contract and namespace keys (gql.*)
  • 3 questions: macros excluded from fallback intentional? SHOW FUNCTIONS/dropFunction asymmetry conscious? Attached-DB scope out-of-design?

@adsharma
adsharma merged commit e1a68c0 into LadybugDB:main Oct 4, 2026
4 checks passed
@chiangchenghsin-hash

Copy link
Copy Markdown
Contributor Author

Thanks for the review and the merge — and for the three questions. Each answer verified against the merged code:

1. Macros excluded from fallback — intentional?
Not excluded from resolution: getFunctionEntry walks local functions → macros → internalFunctions and then recurses into the fallback, which walks the same chain on main — so a main-catalog macro does resolve while the session is on a graph. What is true: containsFunction consults functions/internalFunctions only and never macros — but that macro-blindness predates this patch (unchanged in the diff) and the fallback faithfully mirrors each surface's pre-existing shape. Happy to align containsFunction with getFunctionEntry (local + fallback, macros included) in a small follow-up if you'd like the two to agree.

2. SHOW FUNCTIONS / dropFunction asymmetry — conscious?
Conscious minimalism: the enabler's scope was resolution (the only thing USE GRAPH broke). The other two surfaces keep their local-only semantics on purpose, with one rough edge worth naming: dropFunction resolves fallback-aware but then drops from the local functions set, so for a fallback-resolved name validateExistNoLock throws "does not exist" — confusing message, but safe (the tombstone is name-keyed to the local set; nothing is deleted cross-catalog). getFunctionEntries (SHOW FUNCTIONS) lists the current catalog's functions set only (not even internalFunctions). If you want symmetry, two candidate follow-ups: (a) SHOW FUNCTIONS unions the fallback's entries (dedup by name), (b) dropFunction raises a clear "owned by the main catalog" error (or drops at the owning catalog). Happy to send either — tell us which semantics you prefer.

3. Attached-DB scope — out of design?
Yes. The fallback is wired only when a graph catalog is created (database_manager.cpp), i.e. USE GRAPH within one database. Cross-database resolution would need a name-conflict policy across attached DBs, which we didn't want to invent in a minimal enabler.

Follow-ups you listed

adsharma pushed a commit to LadybugDB/extensions that referenced this pull request Oct 6, 2026
…ql/tck 190/206)

Evolution of this extension's translator into a TCK-verified generation,
building on the existing scaffold (parser vendoring, build system incl.
ANTLR4CPP_STATIC/macOS symbol export, and the original basic.test by
@adsharma — kept and evolved).

- transformer: ground-up rework — explicit per-statement dispatch,
  SELECT/GROUP BY/HAVING, writes, transactions/sessions, schema-namespace
  registry; multi-hop quantified path patterns (bounded expansion, ISO list
  bindings); exact TRAIL/ACYCLIC/SIMPLE path modes (whole-path predicates
  where the engine's *ACYCLIC is weaker); value-model bridges for ANY-graph
  JSON properties (comparison/order/total-order aggregates); narrow
  GQLSTATUS stamping
- loud-rejection policy: unmapped constructs fail fast with
  'GQL feature not supported: <construct>' — zero silent wrong answers
- tests: 161 dual-run parity cases across 18 suites (evolved from
  basic.test); opengql/tck harness with three-tier GQLSTATUS assertions
  and corpus-integrity footnotes — 190/206 green (70 passed + 120
  passed-with-note), 9 loud-rejection failures, wrong-GQLSTATUS=0;
  REPORT.md included
- docs: README (graded compatibility matrix, 23 known semantic
  differences), THIRD_PARTY_NOTICES.md (Apache-2.0 sources the mapping
  rules were adapted from)
- build: +yyjson include + gql_json_functions sources

Requires the engine-side enablers in LadybugDB/ladybug#1104.
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.

2 participants