Repository navigation
fix(inputlayer-js): compile kg.query() to queries the engine accepts - #229
Merged
Merged
Conversation
kg.query() compiled every query to `?Head <- body`, which the engine parser
rejects ("Query cannot contain a rule definition"), so every query example
in the JS SDK guide failed against a real server. The integration tests
used raw IQL, and CI never ran them.
- Plain queries compile to `?atom, body...`, as the Python SDK does. The
engine returns every variable the query binds, so the SDK picks the
selected columns out of the rows. ORDER BY becomes a sort annotation in
the first atom (its relation is moved first), so the engine sorts before
limit/offset. Latency matches the equivalent raw IQL.
- Aggregates compile to a rule and a query in one program; the rule lives
only for that request. Each aggregate gets its own column (CountId,
AvgSalary), topK expands to its passthrough and ordered columns, and an
OR split is aggregated over the union of its branches.
- OR branches are merged client-side: union deduplicated on the atom
variables, then ordered and paginated.
- Non-equality join conditions (e1.id != e2.id) are kept as filters
instead of being dropped.
- New tests/docs-queries.integration.test.ts runs every query example in
js-sdk.mdx against a live engine; `make js-test-live` builds and starts a
server for it, and the full suite runs it in a new job.
- Fix two guide examples the engine could not run: topK per group needs
the kept columns in `passthrough`, and the last-hour filter compares
the timestamp with fn.timeSub() instead of passing fn.withinLast() as a
condition.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Ok continue. (Context: the production sign-off review found a release blocker: the JavaScript SDK's kg.query() compiles every query to '?H <- body', which the engine parser rejects with 'Query cannot contain a rule definition'. Every documented query example in the JS SDK docs (js-sdk.mdx) fails; the integration tests avoid kg.query, so CI never saw it. Findings are fixed before release.)
What Changed
kg.query()no longer compiles to the?Head <- bodyform, which the engine rejects with "Query cannot contain a rule definition". A newcompileQueryPlan()builds the programs to run plus a description of the result's shape:?atom, body...and the SDK pulls the selected columns out of the rows the engine returns.orderBybecomes a:asc/:descannotation on the first atom, and the ordered relation is moved first.CountId,AvgSalary), andtopKexpands to itspassthroughand ordered columns.e1.id != e2.idare kept as filters instead of being dropped.offsetwithout alimitis applied on the client.kg.debug()andkg.why()now read from the same query plan.why()runs a dedicated rule, finds result columns by position, and applies the query's ordering and paging to its rows, provenance and proof trees.tests/docs-queries.integration.test.ts, which runs each query example injs-sdk.mdxagainst a live server. A newmake js-test-livetarget builds and starts that server, and a newjs-sdk-livejob in the full suite runs it;npm testnow skips every*integration.test.tsfile. Two guide examples were fixed so they run: per-grouptopKnow puts the columns to keep inpassthrough, and the last-hour filter now compares againstfn.timeSub(fn.timeNow(), …)instead of passingfn.withinLast()as the condition. Compiler unit tests were updated to match the new output.Risk Assessment
✅ Low: The round-3 fix applies a lone offset in the single-program path of query(). It slices the projected rows after the engine has ordered them. It also slices row_provenance and recomputes rowCount. This matches pageOf, which why() and OR merges already use, so all three paths now give the same rows. The prior rounds' plan/why rework was checked and nothing new was found.
Testing
I built the release server and ranmake js-test-live, which runs every js-sdk.mdx query example plus the debug/why/offset tests against a live engine: all 25 passed. I ran the same tests against the base-commit SDK: 24 of 25 failed with "Query cannot contain a rule definition (<-)", which confirms the blocker existed and is now fixed. An independent ad-hoc script on a separate scratch server also gave correct rows for: plain queries, joins, aggregates, ordering and paging, a lone offset (query() and why() agree), OR merges, aggregate debug plans, why() on second-relation columns, and aggregate limit. It also showed that no session rule was left behind. The scratch servers and temp dirs were removed and the worktree is clean.Evidence: Live js-sdk.mdx docs query suite on the fixed SDK (25/25 pass)
Evidence: Same live suite on base SDK 4adce1e (24/25 fail with 'Query cannot contain a rule definition')
Evidence: Ad-hoc live SDK transcript (query/why/debug/offset/OR/aggregate)
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed (3) ✅
packages/inputlayer-js/src/knowledge-graph.ts:609-kg.debug()(line 609) andkg.why()(line 625) still callcompileQuery()and ignore the newQueryPlan.query()was the only method moved to the plan. Two cases go wrong. (1) Aggregates:compileQuerynow returns a two-line program (il_sdk_agg(...) <- bodyfollowed by?il_sdk_agg(...)). Prefixing it with.debug/.whyonly wraps the first line. The engine runs the second line as a separate query (handler.rs line loop, around line 2504), and that query fails becauseil_sdk_aggwas never registered as a session rule. Example:kg.debug({select:[Employee.col('department').toAst(), count(Employee.col('id'))], join:[Employee]})returns an error instead of a plan. (2) Plain queries:why()returns the engine's raw columns, which are every variable in every atom, not the selected/labelled columnsquery()returns for the same options. OR splits in both methods still use only the first branch, but that was already the case. Neither aggregate debug/why nor why() is documented in js-sdk.mdx, and aggregate debug/why was already broken under the old?H <- bodyform, so this does not block the stated intent. It is a sibling path of the same compile-to-engine contract, though. What needs deciding is the remedy: how debug/why should handle a rule-plus-query program, and whether why() should project to the plan's outputs.🔧 Fix applied.
2 warnings still open:
packages/inputlayer-js/src/knowledge-graph.ts:609-kg.debug()(line 609) andkg.why()(line 625) still callcompileQuery()and ignore the newQueryPlan.query()was the only method moved to the plan. Two cases go wrong. (1) Aggregates:compileQuerynow returns a two-line program (il_sdk_agg(...) <- bodyfollowed by?il_sdk_agg(...)). Prefixing it with.debug/.whyonly wraps the first line. The engine runs the second line as a separate query (handler.rs line loop, around line 2504), and that query fails becauseil_sdk_aggwas never registered as a session rule. Example:kg.debug({select:[Employee.col('department').toAst(), count(Employee.col('id'))], join:[Employee]})returns an error instead of a plan. (2) Plain queries:why()returns the engine's raw columns, which are every variable in every atom, not the selected/labelled columnsquery()returns for the same options. OR splits in both methods still use only the first branch, but that was already the case. Neither aggregate debug/why nor why() is documented in js-sdk.mdx, and aggregate debug/why was already broken under the old?H <- bodyform, so this does not block the stated intent. It is a sibling path of the same compile-to-engine contract, though. What needs deciding is the remedy: how debug/why should handle a rule-plus-query program, and whether why() should project to the plan's outputs.packages/inputlayer-js/src/knowledge-graph.ts:626- This defect is in code that review round 1's fix added (moving why() onto the QueryPlan).why()now passes the engine's.whycolumns throughprojectRows→resultColumnIndexes(compiler.ts:752). That function assumes the result columns follow?querynaming: named by variable, or schema-named and positional over the first atom. The.whypath names columns differently.extract_query_schema/extract_head_variables(src/protocol/handler.rs:9152-9192) splits the raw text between the first '(' and the LAST ')', and labels any column beyond thatcol{i}. Two cases now throwQuery result has no column for …where why() used to return rows. (1) A join that selects a column of the second relation. Example:?employee(Id, …, Active), department(Department, Floor)gives the columns…, "Active)", "department(Department", "Floor". byName is false, so the fallback usesgoalVars.indexOf('Floor'), which is -1. (2) A select of a computed or expression column. Example:?employee(…), Expr = Salary * 2names the extra columncol5. byName is true, socolumns.indexOf('Expr')is -1. A where with a function call, or a string containing ')', also garbles the names. The new live tests cover only a one-relation select of plain columns, so they pass with this bug. Fix: for why(), resolve columns by the known variable order ofplan.explain(the engine's__query__head lists the variables in order of first appearance) instead of trusting the engine's.whycolumn names. Alternatively, fixextract_head_variablesin the engine so.whynames columns the way the query path does. Add live why() tests for a join that selects a second-relation column and for a computed column. Related sibling from the same round: the aggregateexplainrule (compiler.ts compileAggPlan) has no orderBy or limit/offset. Sokg.why({... count(...), limit: 1})returns every group, while query() returns one.🔧 Fix applied.
1 info still open:
packages/inputlayer-js/src/compiler.ts:442-limitAtomemits nothing whenoffsetis set withoutlimit, so a single-programkg.query({ ..., offset: 2 })ignores the offset. That was already the case before this change. The new client-side paging inpageOf(knowledge-graph.ts:927), which round 2 added forwhy()and which this change added for merged OR branches, does apply a lone offset. So the same options give different rows depending on whether thewherehas an OR, andkg.why()disagrees withkg.query(). Fix: pick one meaning for a lone offset. Either emit an offset-only pagination atom, if the engine supports one, or havepageOfignoreoffsetwhenlimitis undefined, which matches the engine path.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
make js-test-live JS_LIVE_PORT=8197(release inputlayer-server on a scratch data dir +vitest run tests/docs-queries.integration.test.ts, 25 live tests)Base-commit reproduction:git archive 4adce1e3 packages/inputlayer-js, then the same live docs-queries suite run on that copy against a scratch server on port 8198Ad-hocvite-node adhoc.tsscript using the new SDK against the scratch server: all rows, filter (with '(' and an apostrophe in the data), join, count/avg aggregate, orderBy+limit, lone offset in query() and why(), OR merge, debug of an aggregate, why() selecting a second-relation column, why()/query() aggregate with limit 1, then.sessionto confirm no il_sdk_agg rule leaked✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.