Skip to content

Register a query and call it by id or by name (Steps 3, 4 and 5 of #490) - #500

Merged
zipdoki merged 8 commits into
mainfrom
feat/prepared-query
Aug 31, 2026
Merged

zipdoki merged 8 commits into
mainfrom
feat/prepared-query

Conversation

@zipdoki

@zipdoki zipdoki commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

The skeleton in #491 declared the endpoints and returned nothing. This fills them in: a query registers once, gets called by id or by a name pointing at one, and takes its values per call.

POST   /graph/v3/prepared-queries            register
GET    /graph/v3/prepared-queries/{id}       read one, by id or by a name for it
PUT    /graph/v3/prepared-queries/{id}
DELETE /graph/v3/prepared-queries/{id}
POST   /graph/v3/prepared-queries/aliases    point a name at an id
POST   /graph/v3/query/{id}                  run it

Registration declares the arguments with their types, and the body holds {name} anywhere a whole value would go.

{
  "name": "top_items_of_friends",
  "arguments": [{ "name": "entity", "type": "STRING" }, { "name": "size", "type": "INT" }],
  "fetch": [
    { "type": "SCAN", "name": "follows", "database": "graph", "table": "follows", "index": "created_at", "source": { "type": "VALUE", "value": "{entity}" } },
    { "type": "TOPK", "name": "ranked", "database": "graph", "table": "orders", "topk": "top_product_groups_1y", "entity": { "type": "REF", "ref": "follows", "field": "tgt" }, "limit": "{size}" }
  ],
  "transform": [
    { "type": "SQL", "name": "top_items", "sql": "SELECT ranked.tgt, SUM(ranked.metric) AS metric FROM ranked GROUP BY ranked.tgt ORDER BY metric DESC LIMIT ?", "arguments": ["size"] }
  ]
}
{ "arguments": { "entity": "user1", "size": 10 } }

Binding happens on the JSON, before it's read into an ActionbaseQuery. It has to: "limit": "{size}" can't be the Int that slot wants until the value is in. A placeholder inside SQL is different — it becomes a ? bound at execution, so the SQL text and therefore the compiled plan stay identical between calls.

Registration checks that the names used and the names declared are the same set, so a typo fails at registration rather than at the first call. POST /graph/v3/query takes the same shape unregistered, with values sent alongside the body — no declaration, so a value goes in as the JSON it came as.

Registered queries and the names for them live in the metastore as sys.query and sys.query_alias. PreparedQueryService caches the compiled query by id and the id behind each name, since resolving a name and re-parsing a template on every call is the cost this whole feature exists to avoid.

Placeholders are whole values only. "user-{entity}" is a literal string, not a template. That keeps arguments typed and keeps me out of building strings from untrusted input. If string composition turns out to be needed, it belongs in the SQL transform rather than in the binder.

Running a query lives on the query path (POST /graph/v3/query/{id}), not under /prepared-queries/{id}, so reading a query and running it never look alike. That's another read on POST, so ReadOnlyRequestFilter learns one more segment (/query/).

The second commit fixes the first. Cache invalidation on delete ran in doFinally, which fires after the completion signal has already reached the caller — so a caller's very next read could still be served the query it just deleted. It runs before completion now. PreparedQuerySpec > a query a name still points at cannot be dropped is
the test that caught it.

There's no versioning. An update replaces a registration in place, and a call already in flight binds against the shape it loaded. I'll leave versioned registrations for a follow-up.

Stacked on #499 — that needs to land first. This supersedes #491; I'll close that one once this lands.

Part of #490.

Test plan

  • ./gradlew :engine:test --tests '*PreparedQueryTest*' — placeholder binding, whole-value rule, declared-vs-used names, type casting, SQL arguments
  • ./gradlew :engine:test --tests '*TransformRequestTest*' — the request shape for fetch plus transform plus arguments
  • ./gradlew :engine:test --tests '*PreparedQuerySpec*' --tests '*TransformQuerySpec*' — registration, names, update, delete, cache invalidation, and a registered query with a transform end to end
  • ./gradlew :server:test --tests '*PreparedQueryE2ETest*' — every endpoint over HTTP, by id and by name
  • ./gradlew :server:test --tests '*ReadOnlyRequestFilterTest*' — running a prepared query passes in read-only mode
  • ./gradlew spotlessCheck build — formatting and full build

AI Assistance

  • This PR was written largely with AI assistance.
    • Tool / model: Claude Code (Opus 5)

@zipdoki zipdoki self-assigned this Aug 20, 2026
@zipdoki
zipdoki requested a review from em3s as a code owner August 20, 2026 04:14
@dosubot dosubot Bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request labels Aug 20, 2026
@zipdoki
zipdoki removed the request for review from em3s August 20, 2026 04:14
@zipdoki zipdoki changed the title Register a query and call it by id or by name (Step 3 of #490) Register a query and call it by id or by name (Steps 3, 4 and 5 of #490) Aug 20, 2026
@zipdoki zipdoki mentioned this pull request Aug 20, 2026
5 tasks done
@zipdoki
zipdoki force-pushed the feat/prepared-query branch from f22f1e5 to 01c9c5a Compare August 27, 2026 08:08
@zipdoki
zipdoki force-pushed the feat/prepared-query branch from 01c9c5a to 5561bcb Compare August 31, 2026 00:51
@zipdoki
zipdoki force-pushed the feat/prepared-query branch from 5561bcb to d6fc292 Compare August 31, 2026 01:03
Base automatically changed from feat/calcite-transform to main August 31, 2026 01:49
@zipdoki
zipdoki force-pushed the feat/prepared-query branch from d6fc292 to 11c373d Compare August 31, 2026 01:49
`isRead` recognised the two read-only POSTs whose path ends in a variable by searching the path for `/topks/` or `/query/`. Both are ordinary names — `^[a-z][a-z0-9_]{0,63}$` accepts either — so a database called `query` put `/query/` in the path of everything under it, and `POST /graph/v3/databases/query/tables` was served in read-only mode.

Match the whole path instead. Both endpoints sit at a fixed depth, so an anchored pattern costs nothing and a future write route under `/graph/v3/query/` no longer passes by accident.
The comment on `prepare` claimed it let a SQL that cannot be prepared be refused at registration. It does not: it rewrites `{name}` to `?` and never reads the SQL. A plan is compiled against the columns of the frames the statement reads, and those are only known once the fetch steps have run, so registration cannot check it — a statement Calcite would reject registers cleanly and fails on its first call.

Say that instead. Preparing at registration is worth having and is filed separately; it needs a fetch step to know its output schema without running.
Binding a call's arguments reached into the transform with `as Transform.Sql`. `Transform` is sealed and `Sql` is the only kind today, so the cast holds — but adding a second one would compile and then throw on the first call that uses it. Every other place that opens a `Transform`, a `PostProcessor` or an `Aggregator` in this executor uses an exhaustive `when`; this was the one that did not.
Dropping a query first checks that no name points at it, and that check read `getAll`, which stops at `metadataFetchLimit` without saying so. Past the limit the scan returns a page that names other queries and the delete goes through, leaving a name pointing at something that is gone.

Refuse on a page filled to the limit instead. `DatastoreTableReferences` reads its own scans under the same rule, for the same reason: the caller is deciding whether to drop, so an absence we cannot back is worse than an error.
The rewriter walks a transform's SQL tracking `'` and nothing else. Inside a string literal that is right, and the comment on it says so, but nothing held the rule down. Inside a `--` comment it is wrong: the braces become a `?` Calcite never parses, and the call is refused on the argument count with a message that mentions neither the comment nor the SQL.

Both are pinned as they stand. The literal is the contract; the comment is a shape worth seeing before it is changed, and the SQL is refused either way, which is the part that matters.
Every test in the class registers the same SQL, so whichever ran first parsed, planned and opened the statement's sessions and the rest read a cached plan. Alone that test took 0.7s against 0.01s for its siblings; under a full build it reached 5.02s and tripped `WebTestClient`'s five second response timeout. Two of three full runs failed that way.

Run it once in setup instead, with a longer timeout on that one call. The default stays where it is for every test, so it still guards the path they all take, and the wait that is left is the cost `CalciteLatencyProbe` measures rather than anything under test. It goes away once a registered query prepares when it is registered.
@zipdoki

zipdoki commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Optimistic Merge

@zipdoki
zipdoki merged commit 30d2a16 into main Aug 31, 2026
3 checks passed
@zipdoki
zipdoki deleted the feat/prepared-query branch August 31, 2026 05:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant