Skip to content

Build query result rows one row at a time - #992

Open
paracycle wants to merge 2 commits into
uk-cypher-api-refactorsfrom
uk-lazy-query-rows
Open

Build query result rows one row at a time#992
paracycle wants to merge 2 commits into
uk-cypher-api-refactorsfrom
uk-lazy-query-rows

Conversation

@paracycle

@paracycle paracycle commented Aug 6, 2026

Copy link
Copy Markdown
Member

TL;DR

Rubydex::Query::Result#each built every row before it yielded the first one. It converts one row at a time now, so first on a 300 row result went from 1516 allocations down to 19. Also, the walk shares one frozen key String per column instead of allocating a key per cell.

This stacks on #991, so it is based on uk-cypher-api-refactors. I recommend reviewing per commit.

The problem

#each called #rows, which built the whole array first. A caller that read one row still paid for all of them, twice: once as CCells in C memory, and once as Ruby objects. first and find were the worst case, since they break after one row and then throw the other 299 away.

What changed

The row iterator is a cursor now. rdx_result_set_rows builds only the column strings, and rdx_rows_iter_next frees the cells of the previous row and converts one row under the graph read lock.

The lock is released before the call returns, so a block can run any code between two rows, including code that writes to the graph. Holding it across rb_yield is not an option, since a block that calls graph.index_source would deadlock the thread.

rows keeps its old behaviour and collects everything through the same cursor, so each still walks the memoized array once it exists.

Allocations for 300 rows and 2 columns, from GC.stat(:total_allocated_objects), measured against a real build of each tree:

Call Before After
result.first 1516 19
result.rows 1506 907

The first row is the streaming win. The second is the shared keys: 600 key Strings disappear.

The stale node check

Since the graph can change mid-walk now, the second commit removes a fallback in build_cell that turned an unresolvable node into a plain String. I hit it while testing the cursor: after a delete_document inside an each block, the remaining rows came back as "Cat" and "Dog" instead of handles, with no signal at all. A column changing type halfway through a result is worse than an exception.

So rdx_rows_iter_next reports MissingNode and the extension raises Rubydex::StaleQueryResultError, which is a Rubydex::QueryError. The message names the node.

The check is narrow on purpose, and it is not mutation detection:

  • It only fires when the graph no longer holds a node that the result returned.
  • It does not fire for a re-index that keeps the ids, since a declaration id comes from the name.
  • It says nothing about a handle that a walk already handed out. Those resolve against the graph on each call, exactly like the handles from Graph#[].

render, columns, size, and empty? keep working after a change, since they read the executed result set and never touch the graph.

I did not add a graph revision counter for this. It would need a change to the graph pointer in graph_api.rs, and it would fire on load_config, set_encoding, and completion, none of which can drop a node.

Verification

  • bundle exec rake test: 356 Ruby runs, 1597 assertions, plus the Rust suite. 0 failures.
  • cargo fmt --check, cargo clippy, and rubocop are all clean.
  • Eight new tests cover the early exit, an Enumerator without a block, a break and a raising block, the shared frozen keys, and the four stale result cases.
  • A retention probe streams 60 rows with node, string, list, and map columns, holds every value, churns the C heap with 20,000 strings, runs GC.verify_compaction_references and GC.compact, and then compares against an eager rows call. Everything matches, so nothing Ruby hands back points into the freed cells.

Tophatting

graph = Rubydex::Graph.new
graph.index_workspace
graph.resolve

query = Rubydex::Query.parse("MATCH (c:Class) RETURN c, c.name")
query.run(graph).first          # converts one row
query.run(graph).render(:json)  # no rows built at all

@paracycle
paracycle requested a review from a team as a code owner August 6, 2026 21:13
@paracycle
paracycle force-pushed the uk-cypher-api-refactors branch 2 times, most recently from 69d6b71 to 460fcfa Compare August 6, 2026 21:46
`Rubydex::Query::Result#each` built every row before it yielded the
first one. A caller that read one row still paid for all of them, twice:
once as `CCell`s in C memory, and once as Ruby objects.

The row iterator is now a cursor. `rdx_result_set_rows` builds only the
column strings. `rdx_rows_iter_next` releases the cells of the previous
row, then converts one row under the graph read lock. The lock is
released before the function returns, so the caller may run any code
between two calls.

`each` now converts one row, yields it, and discards it, unless `rows`
already built the whole array. `first` and `find` therefore stop as soon
as the block breaks. `rows` keeps its behaviour: it collects every row
through the same cursor, freezes the array, and reuses it.

The walk also shares its Hash keys. It builds one frozen UTF-8 String
per column, and every row of the walk uses those keys. `rb_hash_aset`
stores a frozen String key as it is, instead of duplicating and freezing
it, so a wide result no longer allocates a key String per cell.

Allocations for 300 rows and 2 columns, counted with
`GC.stat(:total_allocated_objects)`:

| Call            | Before | After |
| --------------- | ------ | ----- |
| `result.first`  |   1516 |    19 |
| `result.rows`   |   1506 |   907 |

Two contracts change:

- A `CResultRow` from `rdx_rows_iter_next` stays valid only until the
  next call or until `rdx_rows_iter_free`, not until the free alone. The
  C extension copies every value into Ruby objects at once, so it
  complies.
- The graph read lock is no longer held for the whole walk. A block that
  writes to the graph therefore changes how later rows resolve their
  node cells. A declaration that disappeared falls back to its display
  name. Holding the lock across `rb_yield` is not an option, because a
  block that calls `graph.index_source` would then deadlock the thread.

The new tests cover the early exit, an `Enumerator` without a block, a
`break` and a raising block, and the shared frozen keys.
Now that rows are built one at a time, the graph can change in the middle
of a walk. `build_cell` handled that by falling back to a plain String when
it could not resolve a node, which quietly turned a `Declaration` column
into a `String` halfway through a result. I hit exactly that while testing
the cursor: after a `delete_document` inside an `each` block, the remaining
rows came back as `"Cat"` and `"Dog"` instead of handles, with no signal at
all.

So the fallback is gone. `build_cell` returns the node name as an error,
`rdx_rows_iter_next` reports `MissingNode`, `rdx_rows_iter_error` names the
node, and the extension raises `Rubydex::StaleQueryResultError`, which is a
`Rubydex::QueryError`. The `List` and `Map` arms free the cells they
already built before they propagate, so a half-built row leaks nothing.

The check is narrow on purpose, and this is not mutation detection:

- It only fires when the graph no longer holds a node that the result
  returned, or when a node id cannot be decoded.
- It does not fire for a re-index that keeps the ids. A declaration id
  comes from the name, so the same names still resolve while the
  definitions and ancestors behind them may differ.
- It says nothing about a handle that a walk already handed out. Those
  resolve against the graph on each call, exactly like the handles that
  `Graph#[]` returns.

`render`, `columns`, `size`, and `empty?` keep working after the graph
changes, since they read the executed result set and never touch the graph.

I did not add a graph revision counter for this. It would need a change to
the graph pointer in `graph_api.rs`, and it would fire on `load_config`,
`set_encoding`, and completion, none of which can drop a node. The missing
node is the condition that actually matters here.
@paracycle
paracycle force-pushed the uk-lazy-query-rows branch from 05de426 to dd9d491 Compare August 6, 2026 21:55
Comment thread test/graph_test.rb
"MATCH (c:Class) WHERE c.name IN ['Animal', 'Dog'] RETURN c.name ORDER BY c.name",
).run(graph)

enumerator = result.each

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.

Do we need to mark the block as optional here:

rubydex/rbi/rubydex.rbi

Lines 553 to 554 in dd9d491

sig { override.params(block: T.proc.params(row: T::Hash[String, T.untyped]).void).returns(T.self_type) }
def each(&block); end
?

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.

3 participants