graph, store: Avoid using to_jsonb when looking up a single entity #5372
Conversation
|
@lutter is this ready for review? |
|
It is ready for review in the sense that it does what it claims to do, but it introduces a bunch of machinery whose full value we'd only reap if we used it more widely. But maybe it's enough to do this as a first step in that direction. |
|
This pull request hasn't had any activity for the last 90 days. If there's no more activity over the course of the next 14 days, it will automatically be closed. |
lutter
force-pushed
the
lutter/dsl-dyn
branch
2 times, most recently
from
5794366 to
c514b58
Compare
| let bi = self | ||
| .0 | ||
| .to_bigint() | ||
| .expect("The implementation of `to_bigint` for `OldBigDecimal` always returns `Some`"); |
There was a problem hiding this comment.
I guess here the intention was to return an error. Also the comment hints at it.
There was a problem hiding this comment.
It's one of these 'should never happen', but you are right, since this already returns an error, we might as well not panic
| .select_cols(&columns) | ||
| .filter(table.id_eq(&key.entity_id)) | ||
| .filter(table.at_block(block)) | ||
| .filter(table.belongs_to_causality_region(key.causality_region)); |
| let child_table = layout.table_for_entity(first_entity)?; | ||
| let sort_by_column = child_table.column_for_field(&child.sort_by_attribute)?; | ||
| if entity_types.is_empty() { | ||
| return Err(QueryExecutionError::ConstraintViolation( |
There was a problem hiding this comment.
Nice flattening the nested ifs and adding one more correctness check.
Our queries all ultimately get their data by doing something like `select to_jsonb(c.*) from ( ... complicated query ... ) c` because when these queries were written it was far from obvious how to generate queries with Diesel that select columns whose number and types aren't known at compile time. The call to `to_jsonb` forces Postgres to encode all data as JSON, which graph-node then has to deserialize which is pretty wasteful both in terms of memory and CPU. This commit is focused on the groundwork for getting rid of these JSON conversions and querying data in a more compact and native form with fewer conversions. It only uses it in the fairly simple case of `Layout.find`, but future changes will expand that use
lutter
force-pushed
the
lutter/dsl-dyn
branch
from
c514b58 to
605c6d2
Compare
|
Rebased to latest master and addressed review comment |
Roundtripping arrays of enums would fail because we would read an array of
enums back as a single string "{yellow,red,BLUE}" instead of the array
["yellow", "red", "BLUE"]. Storing an update to such an entity, even if
users make no changes to that field, would fail because Postgres expects an
array and we were sending a scalar value.
This fixes a bug introduced in PR
#5372
Our queries all ultimately get their data by doing something like
select to_jsonb(c.*) from ( ... complicated query ... ) cbecause when these queries were written it was far from obvious how to generate queries with Diesel that select columns whose number and types aren't known at compile time.The call to
to_jsonbforces Postgres to encode all data as JSON, which graph-node then has to deserialize which is pretty wasteful both in terms of memory and CPU.This commit is focused on the groundwork for getting rid of these JSON conversions and querying data in a more compact and native form with fewer conversions. It only uses it in the fairly simple case of
Layout.find, but future changes will expand that use