Repository navigation
fix(gfql): DAG bindings consume the root graph, deterministic output order (#1923) - #1927
Merged
Merged
Conversation
…order (#1923) Every binding kind in a `let()` DAG now filters from the DAG's root graph. `execute_node` received the previous binding's result and handed it to `ASTCall` while every other kind re-fetched the root out of the binding namespace, so two independent siblings leaked into each other and reordering two unrelated dict keys changed the answer. The root is now the `g` parameter of `execute_node`, which removes the leak, removes the `__original_graph__` namespace entry that a user binding could hijack (and that `output=` could address), and deletes the four duplicated lookups. `topological_sort` iterated a set of strings, so sibling order — and therefore which binding is returned when `output=` is omitted — varied with per-process string hashing. Ties now break by declaration order; `detect_cycles` sorts its neighbours so the reported cycle path is stable too. Polars declined a traversal over a graph with unbound edge endpoints (a row-pipeline `call()` result, or an edgeless graph) through a bare `assert` with an empty message, surfacing as `RuntimeError: ... AssertionError:`. It is now the same typed `NotImplementedError` the chain surface already raises for the shape. A nested `let`'s free variables are now visible to the scheduler, so lexical read-through no longer depends on declaration order; single-binding DAGs no longer skip dependency validation, so `let({'x': ref('x')})` reports the self-reference message a two-binding DAG already produced; and structural DAG errors carry `GFQLValidationError` codes E151/E153 — the same codes the Cypher GRAPH/USE surface raises for unresolved and circular graph references — instead of bare `ValueError`. `GFQL*Error` from a binding reaches the caller with its type intact rather than downgraded to `RuntimeError`. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
lmeyerov
commented
Aug 15, 2026
| path.append(node) | ||
|
|
||
| for neighbor in dependencies.get(node, set()): | ||
| for neighbor in sorted(dependencies.get(node, set())): |
lmeyerov
commented
Aug 15, 2026
|
|
||
| # Update dependents | ||
| for dependent in dependents.get(current, set()): | ||
| for dependent in sorted(dependents.get(current, set()), key=declaration_order.__getitem__): |
Contributor
Author
There was a problem hiding this comment.
Vectorize, incl use native structure to the df engine ?
…1923 review) Review: the DAG scheduler open-coded `sorted()` at six sites over non-engine structures, under TWO DIFFERENT orderings with nothing naming which was which: `detect_cycles` walked neighbours lexicographically while `topological_sort` walked them in declaration order. Same structure, same purpose, divergent answers, and the difference lived only in an inline `key=` argument. Both now go through `in_declaration_order`, and the ordering the scheduler uses is built by `declaration_order_of`. Names an enclosing scope provides carry no declaration index, so they trail the declared ones ordered by name -- that rule was a docstring; it is now `test_enclosing_scope_names_trail_the_declared_ones_ordered_by_name`. Unifying exposed that the fix was incomplete: `detect_cycles` chose its DFS ROOTS by iterating the `dependencies` dict, so only the neighbour walk was declaration-ordered. Deterministic, but it made the reported cycle start at whichever node happened to be inserted first rather than the one declared first. Roots now use the same primitive, so a cycle is reported from its earliest-declared member. Caught by `test_cycle_is_reported_in_declaration_order_not_name_order`, which failed on the first attempt at this refactor. The two identical `sorted(context.get_all_bindings().keys())` error sites are now `executed_binding_names`. The remaining two `sorted()` calls are display renders of a set inside one error f-string (`missing`, `resolvable`), not a traversal contract, so they stay inline and alphabetical for reading. No engine data is sorted anywhere in this file: these are sets of binding-name strings in a `let()` block, and `chain_let.py` schedules operations rather than touching frames. There is no vector to sort. Mutation-verified: restoring the lexicographic neighbour walk and the dict-order root loop turns `test_cycle_is_reported_in_declaration_order_not_name_order` red; restored, green. 181 passed across the let/DAG/call/matcher suites; ruff clean; type-hygiene guard no growth; mypy shows only the known pre-existing polars-skew errors. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
lmeyerov
added a commit
that referenced
this pull request
Aug 15, 2026
113 added lines -> 22, in a file that is always in context. The removed bulk was narrative, not instruction: a seven-item root-cause essay on why the class kept escaping, a three-bullet account of concurrent human review, and two tables whose rows restated each other. Writing a verbose essay about not being verbose is the failure it describes. What survives is every actionable verdict: what to delete from a diff and what carries it instead, the typing verdicts, and the pre-push gate. The root-cause material is dropped rather than relocated -- `bin/ci_comment_density_guard.py` now enforces those three checks mechanically, so prose explaining why humans miss them is no longer the control. The documented gate was wrong as written and is now verified. It fired on `from typing import ... cast ... Any` and on test-file section dividers, so in practice it returned noise and would have been ignored. Scoped to non-test sources with import lines excluded: it returns NOTHING on #1927's diff and 93 hits on #1895's -- a real positive control rather than a grep that is always silent. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
lmeyerov
added a commit
that referenced
this pull request
Aug 15, 2026
…1924/#1926/#1894) Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi
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
1 participant
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.
Independent of the GFQL stack — based on master, merges on its own.
Three silent-wrongs on the
let()/ref()/call()surface, all found by round-012 probing, all cases where both engines agree and both are wrong (differential testing could not have found them).F2 — an
ASTCallbinding consumed the previous binding's output. Every binding kind exceptASTCallre-fetched the root back out of the namespace;ASTCallused the threadedaccumulated_resultas-is. So reordering two dict keys changed the answer —get_degreesreported every person as degree 0 when a filter binding preceded it. Fixed structurally rather than by patching the call site:execute_node'sgparameter now is the root, and all four namespace lookups are deleted.F1 — the default output was nondeterministic across processes.
topological_sortiterated aSet[str]; with per-process hash randomization, master produces three distinct answers across seeds. Ties now break by declaration order, and cycle reporting sorts its neighbours so error paths are stable too.F3 — a user binding named
__original_graph__hijacked the root. Fell out of F2: the root is no longer stored in the user namespace, so that name is an ordinary binding andoutput='__original_graph__'is not addressable. Thestartswith('__')filter in the output error is also gone — it was hiding user bindings.F4 replaces a bare empty-message
assertwith a typedNotImplementedErrornaming unbound edge endpoints, matching how the chain surface already declines the same shape (it also covers an edgeless graph, which died on that assert). F5–F8: nested-letdependencies now resolve viafree_variables(); the single-binding validation shortcut is gone;GFQLValidationErrorpropagates unwrapped instead of being downgraded toRuntimeError; and structural errors use theE151/E153codes the Cypher parser already uses instead of bareValueError.Two places where fixing one exposed another
F7 broke lexical read-through, and F5 explained why. Removing the single-binding shortcut made a nested
letvalidate its own bindings, so an outerrefread as undefined — the shortcut had been masking that, and a two-binding inner let was already broken at master. Fixed by threading enclosing scope through validation and intersecting deps with the local scope before scheduling.F2 exposed a test that pinned the bug.
test_call_operations.py::TestCallInDAG::test_call_in_dagassertedlen(result._nodes) == 3 # Only 'user' nodes— i.e. that a call binding inherits the sibling's filter, precisely the behavior #1923 calls silent-wrong and thattest_let_matchers.py::test_matchers_operate_on_root_graphcontradicts. Rewritten as two named contract pins with a hand-computed degree oracle.Anti-vacuity: 38 of 48 new cells fail at master
test_let_binding_contracts.py. The 10 that pass at master are reported honestly: two hash seeds happen to give the right answer, and the sibling-isolation cells were never wrong. F1's pin runs the DAG in subprocesses acrossPYTHONHASHSEED0/1/2/3/7/13 — the existingtest_execution_order_deterministiccannot see the bug because it never leaves the process.Gates
graphistry/tests/computemaster102 failed, 9340 passed→ branch102 failed, 9391 passed, failure sets byte-identical. Let/scoping suites 140 passed;test_call_operations.py27 passed; polars chain/hop/row-pipeline + new file 1588 passed. ruff, mypy (328 files), both guards clean.Two earlier baseline runs were discarded as contaminated (one ran inside the worktree being edited; one had the new test file temporarily present in the master worktree, tripping the polars lane-completeness lock) and re-run in a dedicated clean worktree.
Self-review gate: zero added comments in
graphistry/compute/**, zeroAny/type: ignore/hygiene-ok/cast(— it also removed a pre-existingcast(Plottable, ...)on a touched line in favour of anis not Nonenarrowing.NIE-tier deliberately left (reported, not fixed): undeclared schema effects,
collapseleaking an internal column, docstrings advertising an idiom pinned to return empty,circle_layout's rawKeyError.🤖 Generated with Claude Code
https://claude.ai/code/session_01MF7uRZLKZaD6Q9FGWSmyXi