Subgraph composition sql more entities

[zorancv]

Add support for querying more entities with a single SQL query.

[lutter]

I don't fully follow how this loads entities and classifies them as create/modify/delete, partly because the interplay of mutable/immutable and is_upper_range is very intricate. I am not sure if code changes can clarify that (would be great); if not, this needs more comments explaining what is going on.

[lutter]

I am not sure if this makes sense, but might be better to stick the entity into the operation, i.e. to have Create(Entity), Modify(Entity), Delete(Entity); that way, users of this enum are forced to consider each of these cases, and it would make it easy to, e.g., change to Delete(Id), i.e., not load the entire entity if that's useful.

But I don't understand how all of composition fits together, so I am not sure if this suggestions is really an improvement.

[zorancv]

I guess here the naming is poor. The enum EntitySubgraphOperation is a kind of operation, with definition more akin to the C style enums. Where the struct just bellow it EntityWithType contains the whole 'entity event' with all the fields including this operation kind and the entity itself.

[zorancv]

I did some renaming which I believe is closer to the semantics. I done it in a separate PR in order to save on rebasing.

[lutter]

There should be a comment explaining what the bool is for

Actually, since this is to indicate whether we want to compare with the upper or lower of the block range, it might be worth having an enum { Lower, Upper } for this

[zorancv]

Yeah. Enum seems reasonable here.

[lutter]

This could just push the bind param block + 1

[zorancv]

I wasn't able to make the compiler borrower happy as the addition creates a temporary variable that can't outlive it's block...

[lutter]

I find this very confusing. It would help if the comment described what the SQL text is that can be generated by this method

[zorancv]

Added some sensible comment. Hope it's clear now.

[lutter]

The body of this is pretty intricate. There should be a comment about what the strategy here is and how it works.

[lutter]

This whole chain of if .. else would probably be clearer if it was turned into a match (lower_now, upper_now) { .. } as that's more symmetric than nested ifs

[zorancv]

I did it with a couple of matches. Not sure that it's more readable, but for sure one level of block nesting is spared and is probably more idiomatic too.

[lutter]

For mutable tables, what is block_number set to? (Looking at the code below, I think I can work it out, but would be nice to have it in the comment)

[lutter]

Does this work? If we have clauses in the query, it returns (text, jsonb, int), but this fallback just returns (int) and is therefore the wrong shape

[zorancv]

I believe I tested it. The intention was to return an empty set. Now when I run it on the command line it returns one row, so maybe diesel silently substituted wrong types with empty rows, but its a bad style for sure. Needs fixing.

[zorancv]

Addressed it with a query that has the same structure and added a test for that case.

[lutter]

Is this left over from something else? I don't see a clause id = $1 being generated.

[zorancv]

@lutter I believe that the comments in the find_range() make the matching algorithm for detecting the type of the operation more clear. Not sure if there is an easy way to make it simpler. Also I feel that splitting mutable and immutable cases won't improve things much, as the immutable case is a special case (and simpler one) of the mutable one.

[lutter]

Not a big deal, but you can match on nested data, too, like:

match self {
    EntityBlockRange::Mutable((_, BoundSide::Lower)) =>  out.push_sql(" lower(block_range) "),
    EntityBlockRange::Mutable((_, BoundSide::Upper)) => out.push_sql(" upper(block_range) "),
    EntityBlockRange::Immutable(_) => out.push_sql(" block$ ")
 }

[lutter]

This helps, but somehow buries the lede: lower_vec contains all entities that were created or updated in block_range

[lutter]

Similarly, upper_vec contains all entitites that were deleted or updated, but will have the previous versions, i.e. in the case of an update, it's the version before the update, and lower_vec will have a corresponding entry with the new version.

[zorancv]

I'll just copy this text.

[lutter]

Why not match on match (lower_now, upper_now) and match arms will then look like (Some(lower), None) etc. I think that would also get rid of the explicit lower and upper variables

[zorancv]

That's beautiful! Did it.

[lutter]

I don't quite understand the sentence it also helps ... I think it should also be mentioned somewhere that this algorithm relies on the fact that lower_vec and upper_vec are ordered by (entity type, entity id, block) which allows for mergesort-like processing of the two vecs.

[zorancv]

I did rewording. Hope it's clear now.

[lutter]

What happens here if lower_vec contains multiple entries for the same entity? Advancing upper_now might move us to the next entity, and so we think that lower_now which has another version of the entity we just looked at will look like a create.

[lutter]

I wonder whether a better strategy for all this might be to throw upper_vec and lower_vec into one vec, sort that by (entity type, entity id, block) and scan the resulting vec. You'd emit an EntityWithType if block is in the desired block range, and the operation will be determined with

• if previous entry had different type/id, it's a create
• if next entry has different type/id, it's a delete
• otherwise it's an update

That sorta requires a 3 element window to pass over the Vec, but since it's a Vec, that could be done with just using indices, like vec[i-1], vec[i], vec[i+1] with some special care for i=0 and i=vec.len()-1, i.e. best to hide those intricacies in helper functions.

[zorancv]

Not sure it it's possible to have multiple modifications of the entity in a block. That would entail a range of a type [10,10). In that case we might end up with wrong modification since we would have ranges like [3,10), [10,10), [10,15) and there is no guarantee that [10, 10) will be before [10,15). I guess if that's the case the DB request has to be extended with a 4. sorting criteria (for the lower_vec the end of the range, and for the upper_vec the start I believe)... Still if there are more than two changes in a block the order of the inner ones is not guaranteed to be correct, only the final result for a block...

[zorancv]

Not sure if merging the two vectors would make the algorithm simpler. It would entail having double entries for each entity that is modified. Also likely we would need a flag if it's a start or end of a range in order to deduce if there is a creation or deletion for single entries, also to avoid false creations at the first entry and false deletion at the last one. And to deduce the proper entry for update. Probably with that flag and 3 element window one can create such algorithm, but I believe it would have at least 6 checks if not more, where the current one has 5. Not to mention the additional complexity of the start/end boundaries in the helper function and the additional complexity needed for the SQL query.

[lutter]

One other thing I am wondering about with this strategy: if you are looking for changes in [10,20) and you find an entity version with a range of [11, 200) will this properly check if there is a version [200,_) to tell whether this is a delete or an update?

[zorancv]

One other thing I am wondering about with this strategy: if you are looking for changes in [10,20) and you find an entity version with a range of [11, 200) will this properly check if there is a version [200,_) to tell whether this is a delete or an update?

We don't reason about the situation around 200 since it's > 20. We only check if there is another entry ending at 11 in order to decide if it's creation or modification at 11.

[lutter]

One thing that confuses me about the sort order (block_number, entity type, entity id) is that entries for different entities can be interleaved, like if there are two entities a and b, you might end up with [ (2, a), (2,b), (3, a), (3, b)] and since find_range processes the entries in the vecs in order, can't you end up comparing an entry for a with one for b?

[zorancv]

You have to keep in mind that matching is always done between elements in the different vectors, and never between elements in the same one. Also a missing element in upper_vec is a creation and missing element in lower_vec is deletion, where a match (a presence of the equivalent respective values in both vectors) is a modification. And finally the two pairs of cases where there is either no matching element on one side or no remaining element at all on that same side, are in essence equivalent and produce the same result. Advancements of iterators are done on the already evaluated element(s).

Probably a simple example would be more explanatory.
Imagine you have elements in lower_vec like this:
[ (2, a), (2, c), (3, a) (3, b), (3, c) ],

and in the upper_vec something like:
[ (2, a), (2, b), (3, a), (3, c), (4, a) ],

the strategy will first find a match of (2, a) in both vectors so modification of a in block 2, then (2, b) is missing in lower_vec so deletion of b in 2, then (2, c) is missed in upper_vec so creation there, then (3, a) matched again so another modification, (3, b) only in lower_vec - creation, (3, c) on both sides - modification, (4, a) only in upper_vec - deletion.

Looking again at the comments at the start of the big while loop I tried to enter the same content as the first paragraph here, but also referred to some other aspects and that might had made it less readable. Should I try to improve it?

[lutter]

Had a discussion with Zoran, and since I have been unable to come up with counterexamples to support my worries, this is good to go.