Make Cypher query execution return a Result instance - #991
Conversation
|
Should this be two separate PRs (one for errors, one for render)? |
I could split, but it felt natural to add the new exceptions while making the API change. |
| return data->rows; | ||
| } | ||
|
|
||
| struct CRowsIter *iter = rdx_result_set_rows(data->result_set, rdxi_graph_from_object(data->graph_obj)); |
There was a problem hiding this comment.
One concern with lazy materialization: rows converts node cells by looking them up in the current graph state. If the graph changes after query.run(graph) but before the first result.rows, the executed result can change shape. For example:
result = Rubydex::Query.parse("MATCH (c:Class {name: 'Dog'}) RETURN c").run(graph)
graph.index_source("zoo.rb", "class Cat; end\n", "ruby")
graph.resolve
result.render(:json) # still has the Dog node from the executed result set
result.rows # [{ "c" => "Dog" }], no longer a Declaration handleCan we materialize the row cells at execution time, or have the result set carry the node category/kind/id captured during execution, so Result is independent of later graph mutations? I'd add a regression test for run; mutate graph; rows too.
There was a problem hiding this comment.
I've gone the other direction and implemented proper lazy materialization here which also checks for node invalidation too. Is that enough to address your concern?
There was a problem hiding this comment.
Looks like Result#render and Result#rows disagree based on graph mutations after execution. Do we need to rebase on #992?
There was a problem hiding this comment.
On the contrary, #992 is based on this branch, since it need this refactor to operate.
0f586bd to
69d6b71
Compare
`Rubydex::Query#render(graph, format)` ran the query and formatted the
result in one call. This commit removes it. The new shape is:
```ruby
query = Rubydex::Query.parse("MATCH (c:Class) RETURN c.name")
result = query.run(graph)
puts result.render(:json)
```
`Query#run` now returns a `Rubydex::Query::Result` instance. That object
owns the executed result set, so `render` never runs the query again.
The class is `Enumerable` over its rows. It answers `columns`, `rows`,
`each`, `size`, `length`, `empty?`, and `render`.
Moreover, the query API now raises specific errors instead of
`ArgumentError`:
- `Rubydex::QueryError` is the base class.
- `Query.parse` raises `Rubydex::QuerySyntaxError` on invalid Cypher
queries.
- `Query#run` raises `Rubydex::QueryExecutionError` when the query fails
against the graph.
`ArgumentError` remains for true argument faults, such as an unknown
output format.
The layers change as follows:
- `rubydex`: `run_parsed` becomes `render(&ResultSet, OutputFormat)`, so
a caller executes once and formats as often as it needs.
- `rubydex-sys`: `rdx_query_run_rows` becomes `rdx_query_execute`. The
new `rdx_result_set_format`, `rdx_result_set_rows`,
`rdx_result_set_column_count`, `rdx_result_set_column`,
`rdx_result_set_row_count`, and `rdx_result_set_free` work on the
returned result set. `rdx_query_run` is deleted. `CQueryErrorKind`
carries the failure kind across the FFI boundary.
- `ext/rubydex/query.c`: adds the `Rubydex::Query::Result` typed data.
It marks `graph_obj` and `rows` movable and implements `dcompact`, so
a compaction can relocate them.
`Rubydex::Query::Result#rows` builds the row objects on the first call
and freezes the array. `Query#run` wraps the Ruby object before it runs
the query, so the result set always has an owner that frees it, even
when execution raises.
69d6b71 to
460fcfa
Compare
|
|
||
| // A result can only be obtained from `Query#run`; `new` would create an object with no Rust | ||
| // data behind it. | ||
| rb_undef_method(rb_singleton_class(cQueryResult), "new"); |
There was a problem hiding this comment.
How? This isn't making the method private, it is completely removing it. How can we remove methods in RBI files?
There was a problem hiding this comment.
I thought we can hide it by making Result.new private? Hide as using Sorbet to prevent users from calling it.
There was a problem hiding this comment.
I am not sure that I like that as a solution, since it only makes it private and doesn't mean it can't be called from other contexts:
class Bar
class << self
private
def new = super
end
end
class Baz < Bar
(Instance = new) rescue puts "Error"
puts "No error when creating: #{Instance}" if defined?(Instance)
end
Bar.new rescue puts "Error"
Baz.new rescue puts "Error"gives:
❯ ruby bar.rb
No error when creating: #<Baz:0x0000000121aec870>
Error
Error
On the other hand, undef_method :new completely removes it:
class Bar
class << self
undef_method :new
end
end
class Baz < Bar
(Instance = new) rescue puts "Error"
puts "No error when creating: #{Instance}" if defined?(Instance)
end
Bar.new rescue puts "Error"
Baz.new rescue puts "Error"gives:
❯ ruby bar.rb
Error
Error
Error
This addresses two review comments on the Cypher blog post in rails-at-scale#431:
ArgumentErroron a syntax error should be a more specific errorrendermixes execution and rendering; the API should bequery.run(graph).render(:json)Execution and rendering are separate
Rubydex::Query#render(graph, format)is gone. The new shape is:Query#runreturns aRubydex::Query::Result. That object owns the executed result set, sorendernever runs the query again. It isEnumerableover its rows, and it answerscolumns,rows,each,size,length,empty?, andrender.Specific errors
Rubydex::QueryErrorRubydex::QuerySyntaxErrorQuery.parse, on invalid Cypher.Rubydex::QueryExecutionErrorQuery#run, when the query fails against the graph.ArgumentErrorremains for true argument faults, such as an unknown output format.The Rust FFI reports the failure kind through a new
CQueryErrorKind, so the C extension raises the matching class.Verification
bundle exec rake test: 300 Ruby runs and the full Rust suite. 0 failures.cargo fmt --checkandrubocop: clean.cargo clippyreports no new warnings.Rubydex::Query::Resultmarksgraph_objandrowsmovable and implementsdcompact, soGC.verify_compaction_referencesfollowed byGC.compactkeeps every reference valid.Follow-up
The blog post in rails-at-scale#431 still describes the old API. It needs the two snippets updated once this merges.