Skip to content

feat(gql): TCK-verified transformer rework (161 dual-run cases, opengql/tck 190/206) - #93

Merged
adsharma merged 9 commits into
LadybugDB:mainfrom
chiangchenghsin-hash:feat/gql-tck-verified-translator
Oct 6, 2026
Merged

adsharma merged 9 commits into
LadybugDB:mainfrom
chiangchenghsin-hash:feat/gql-tck-verified-translator

Conversation

@chiangchenghsin-hash

Copy link
Copy Markdown
Contributor

Rework of this extension's GQL→Cypher translator into a TCK-verified generation. This builds on the existing scaffold (parser vendoring, build system incl. ANTLR4CPP_STATIC / macOS symbol export / grammar regen script, and the original basic.test — all kept and evolved; scaffold originally contributed by @adsharma).

Tracked in LadybugDB/ladybug#1096.

What's inside

  • Transformer ground-up rework (gql_transformer.cpp, ~+10k lines vs the previous generation): explicit per-statement dispatch; SELECT/GROUP BY/HAVING; writes (INSERT/SET/DELETE/REMOVE with order-independence snapshot); 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 than GQL semantics); value-model bridges for ANY-graph JSON properties (comparison/order/total-order aggregates: _gql_lt/le/gt/ge/eq/ne, _gql_sortkey, _gql_max/min/sum/avg, _gql_to_json); narrow GQLSTATUS stamping.
  • Loud-rejection policy: unmapped constructs fail fast with GQL feature not supported: <construct> — zero silent wrong answers. 23 known semantic differences are documented in the README (graded compatibility matrix).
  • Tests: 161 dual-run parity cases across 18 suites — every case runs the GQL statement and an equivalent hand-written Cypher against the same expected result (basic.test evolved into this suite).
  • opengql/tck harness (test/tck/): corpus vendored & frozen; three-tier GQLSTATUS assertions; corpus-integrity footnotes. Result: 190/206 green (70 passed + 120 passed-with-note), 9 failed (all loud rejections: 4 corpus parse-error issues + 5 design rejections), 7 skipped, wrong-GQLSTATUS = 0. REPORT.md included.
  • Docs: README.md (graded matrix + known semantic differences), THIRD_PARTY_NOTICES.md (Apache-2.0 sources the mapping rules were adapted from, e.g. Neo4j Cypher front-end rewriters — registered per file; no GPL/BSL code).
  • Build: +1 include line (yyjson for the value bridges) + gql_json_functions sources; your parser CMakeLists / symbol export lists / grammar regen tooling are untouched.

Testing

  • Windows/MSVC: 161/161 dual-run; TCK 70/120/9/7; full engine regression 1972/1975 (3 offline INSTALL cases in a no-OpenSSL build config).
  • Linux (ubuntu-latest, gcc, -Werror): same gates green — 161/161, TCK classification identical (70/120/9/7), full engine regression 1975/1975, 0 failures. Run: 37164343158.
  • The TCK report generation is deterministic (sorted listings; byte-stable across reruns).

Dependency

Requires the engine-side enablers in LadybugDB/ladybug#1104 (catalog function fallback, extension data slots, path variable alias). This PR's CI will stay red until #1104 lands in the core this repo builds against.

License

MIT, consistent with this repo. THIRD_PARTY_NOTICES.md travels with the code (Apache-2.0 obligations for adapted mapping rules).

…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.
@adsharma

adsharma commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Thanks for the large, well-documented contribution — the loud-rejection policy, dual-run parity suites, and disclosed REPORT.md methodology are the right architecture. I did a read-only review (no compile/test) for correctness, security, and maintainability. One crash-grade blocker plus several majors below; requesting changes on those.

Blocker

_GQL_IS_SIMPLE has no runtime type guard — crash/UB on direct calls (gql/src/function/gql_json_functions.cpp:1645-1656)

DASSERT(0 == StructType::getFieldIdx(input.dataType, InternalKeyword::NODES));
auto nodesVector = StructVector::getFieldVector(&input, 0).get();

DASSERT evaporates in release, and StructVector::getFieldVector bottoms out in reinterpret_cast in release. The function is registered as a public scalar with an ANY parameter (gql_extension.cpp:42-43), so RETURN _GQL_IS_SIMPLE(1) from any untrusted query reaches the reinterpret path. Please add a runtime check that the input is a path STRUCT (with NODES LIST-of-NODE + ID field) and throw RuntimeException otherwise, consistent with the file's loud-reject policy. The sibling flat/unflat sel-vector handling at :1659-1661 is worth hardening at the same time.

Majors

1. gql/test/test_files/test_list is unusable as committed (:1-418)
Every line is an absolute author-machine Windows path, e.g.:

basic.GqlMatchReturn C:/Users/chian/Documents/trae_projects/ladybug-0.21.1/extension/gql/test/test_files\basic.test

Plus: 418 lines but only 113 unique (305 exact duplicates, e.g. basic.* 44x for 11 -CASEs); 54 of 161 -CASEs missing (all of listguard, multihop (14), qpibind (8), relname (6), schemapath (12), smallmodes (8), plus strays); 5 phantom case IDs that exist nowhere else (EqualityUntouchedPin, UnsupportedDifferentEdgesMatchMode, UnsupportedQuantifiedPathPattern, UnsupportedSimplePathMode, VerifyReplyProbeMain / missing verifyreplyprobe.test). If the runner consumes this file, most new coverage never executes; if it doesn't, please delete or regenerate it — don't merge as-is. Same leak class as run_tck.py:62-78 author-machine fallback paths.

2. Harness can go green on total failure (gql/test/tck/run_tck.py:1118-1135, :1354)
n_pass = 0 fallback with no check that any test ran; an e2e_test crash or unparsable log yields failed=∅ → n_fail=0 → exit 0. Please assert n_total == len(index) (minus probe-skips) and return 1 when the gtest footer is absent.

3. Aggregate vs predicate number ordering disagree (gql_json_functions.cpp:267-295 vs :1004-1110)
_GQL_MAX/_GQL_MIN on JSON fold mixed int/real and >int64 comparisons through double (the code's own 2^53 comment), while _GQL_LT/LE/GT/GE/EQ/NE + _GQL_SORTKEY use exact arbitrary-length decimal normalization. _GQL_MAX(json_col) can return a value _GQL_GT/ORDER BY _GQL_SORTKEY considers smaller. Please route JSON min/max number comparison through the same compareDecimalTexts path.

4. _GQL_SORTKEY throws where predicates succeed (:1401-1413)
MANTISSA_WIDTH = 40 / exponent cap throws in ORDER BY on data WHERE accepts, so the same stored value breaks availability depending on entry point. Either widen/length-prefix the mantissa encoding or reject consistently at both layers.

5. _GQL_TO_JSON missing non-finite guard (:48-69 vs :1173-1178)
Siblings throw "non-finite value cannot be ordered" via realToNumberText; _GQL_TO_JSON goes straight jsonify → jsonToString on possibly-NaN/±Inf doubles. Please reject non-finite loudly there too (and audit the typed-DOUBLE dblSum write path).

6. Delimited labels/aliases emitted verbatim (gql_transformer.cpp:2013, also :2896,2988,3024,2005)

head += ":" + sourceText(expr); // Simple label: keep the pattern spelling

MATCH (n:"My Label") emits :"My Label" — invalid Cypher that fails downstream with a misleading syntax error, which is exactly the failure mode the header forbids ("must never reach the Cypher parser"). Same gap for AS "weird alias" and element variables. Please normalize delimited identifiers to backtick-quoted Cypher at these emission points.

Happy to re-review. Suggest merge gates: blocker + items 1–2; items 3–6 can be same-PR fixes or tracked follow-ups, but please don't defer 3–4 silently since predicate/aggregate/sortkey divergence is user-visible. Full read-only finding list (including minors: ;-split false rejection at transformer:1320, ACYCLIC (n) edge case at :2133, fixed-{n} up to 1000 bypassing width caps at :906,929, recursive JSON descent depth, jsonEscape control chars, addFunc silent shadowing, note-tier weakness of 120/190 "green") available on request.

@adsharma
adsharma self-requested a review October 6, 2026 00:42

@adsharma adsharma left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting the changes in the previous comment

chiangchenghsin-hash and others added 8 commits October 6, 2026 13:22
…rd, exact-decimal min/max, long-number sortkey, to_json non-finite, delimited idents)
… green-on-total-failure), portable e2e discovery, REPORT links opengql/tck#9
… green-on-total-failure), portable e2e discovery, REPORT links opengql/tck#9
…cimalTexts), uncapped self-delimiting sortkey, non-finite to_json guard
… stale machine-local cache, runner regenerates it
…elimited-ident funnel), byte-exact copies of run_tck/REPORT/json_functions

The API-upload path truncated gql_transformer.cpp at ~600 lines and introduced
harmless transcription drift in three files (quote escaping, one trailing
space). This commit restores the exact gated tree (self-test 166/166, TCK
70/120/9/7).
@chiangchenghsin-hash

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough read-only review — and for the sharp blocker find. Everything is addressed on the branch; final tree is in 850fdab (the earlier "part N" commits are transport splits, one of them re-stores the full gql_transformer.cpp after an API upload truncated it mid-file — the PR diff is what matters). Summary by item:

Blocker — _GQL_IS_SIMPLE runtime guard ✅

Added ensurePathNodeIDLayout: the input type is validated at runtime (physical STRUCT → NODES LIST-of-NODE → ID field at layout position 0 of type INTERNAL_ID, matching UnaryPathExecutor::executeNodeIDs), and non-path input throws RuntimeException — RETURN _gql_is_simple(1) now fails loudly instead of reinterpret-casting in release. Field vectors are looked up by name after the check. The flat/unflat sel-vector pairing is hardened too: single-row input broadcasts, 1:1 pairs otherwise, any other mismatch throws instead of reading OOB. Pinned in smallmodes.test (IsSimpleTypeGuardReject).

Majors ✅

  1. test_list — removed from the tree and gitignored. You were right on every count (absolute author-machine paths, 305 duplicates, 54 cases missing, 5 phantom IDs): it's the e2e runner's generated case index (written by --gtest_list_tests / full scans) and never should have been committed. Coverage is unaffected — a cache miss falls back to a full directory scan (e2e_test.cpp: findTestFile → scanTestFiles). The run_tck.py author-machine fallbacks are gone too: E2E_BIN is the contract, with a documented probe of standard build layouts only.
  2. Harness integrity — hard gates now: missing gtest footer (crash/unparsable log) → exit 1; executed count ≠ generated count → exit 1; classification incomplete (pass+fail+note+probe-skip ≠ executed) → exit 1. A total-failure run can no longer report green.
  3. min/max vs predicates — _GQL_MAX/_GQL_MIN number comparison now routes through the same exact compareDecimalTexts path as _GQL_LT.._GQL_NE and _GQL_SORTKEY (integers keep exact spelling; a real carries its double's shortest round-trip form — one shared ceiling, one shared order). jsonNumAsDouble is gone. Pinned: MinMaxExactDecimalParity (int 2^53+1 vs real 2^53 — the old double fold tied them).
  4. _GQL_SORTKEY caps — removed entirely: the key accepts exactly what the predicates accept. Mantissa is now self-delimiting (terminator 0x00 positive so shorter-prefix sorts first; 0xFF negative after 9's complement so shorter-prefix sorts last — the two directions need opposite terminators) and the exponent field covers the full int64 range. Pinned: SortKeyLongNumberParity (40+ digit mantissas order correctly through GQL ORDER BY and the direct key).
  5. Non-finite — _GQL_TO_JSON rejects NaN/±Inf loudly (including list/array/map/struct nesting), matching realToNumberText's siblings. Auditing the typed dblSum write path as you suggested found the same gap: typed FLOAT/DOUBLE _GQL_SUM/_GQL_AVG results now reject non-finite instead of writing IEEE inf/nan (the JSON result path already did). Pinned: ToJsonNonFiniteRejected.
  6. Delimited identifiers — normalized at a single funnel at the Transform exit: GQL "..." identifiers (GQL "" escapes) become backtick-quoted Cypher (` escapes), quote-state aware (single-quoted strings and accent-quoted identifiers pass through). One funnel instead of per-site fixes so labels, result aliases and expression references all convert under one rule. Pinned: DelimitedIdentParity. Two honest boundary notes: (a) delimited element variables never reach translation — the vendored GQL.g4's bindingVariable is regularIdentifier-only, a parse-level ceiling we can't touch (grammar frozen); (b) long numeric literals written through CREATE {...} fold through double at the engine's storage layer before this layer sees them — the pins carry exact long numbers via string values (stored raw) and in-expression CAST(... AS JSON).

Also (from the #1104 review notes)

  • rewriteFunc coverage is recorded here explicitly: the per-statement reset is exercised end-to-end by this suite's multi-statement batches.
  • The dataMap contract is documented at our call site (gql_function.cpp): keys namespaced gql.* (slot gql.graphTypes), unguarded map treated as single-threaded-init state inside one connection-serialized CALL GQL. Happy to mirror that into extension_manager.h in a tiny comment-only follow-up to core if you'd like it on the API.

Gates (re-run on this tree)

Could you post the minor findings list whenever convenient (you mentioned ;-split rejection, ACYCLIC (n), fixed-{n} width caps, recursive JSON depth, jsonEscape control chars, addFunc shadowing, note-tier weakness)? Happy to take them in this PR or as tracked follow-ups per your preference. Ready for re-review.

@adsharma

adsharma commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

Thank you for the contribution!

@adsharma
adsharma merged commit 571333a into LadybugDB:main Oct 6, 2026
2 checks passed
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