-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: query filters #4833
refactor: query filters #4833
Conversation
6d4f55f
to
bfb2f31
Compare
Are projections smt we can introduce later? |
Yeah, adding projections in the future is possible. Would also shave off a few specialized queries we have. Although I haven't really been thinking about possible designs. |
Degradation in PK quires might introduce overhead into permission checks in the executor, so we should be careful with it. |
Can you describe briefly describe idea behind new predicates? Like i can see now that we have What are stages predicate go though? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that now quires are handled more uniformly between client and wasm.
Also that now each query is coupled with specific filters is convenient as well.
I've pushed some docs, including explanations of this topic. You would probably want to take a look at |
I think that we should get rid of singular queries. We should have queries that return multiple results but can be filtered to return just one. Would you agree? Edit: I see that you mention this can be done in a separate PR. I agree |
I've already removed all singular queries that can be implemented as a query + filter. Some of them can be removed (by introducing projections into the query system), but others, like |
3b0f875
to
5047a82
Compare
client/tests/integration/smartcontracts/create_nft_for_every_user_trigger/src/lib.rs
Outdated
Show resolved
Hide resolved
client/tests/integration/smartcontracts/executor_custom_data_model/src/complex_isi.rs
Outdated
Show resolved
Hide resolved
client/tests/integration/smartcontracts/executor_custom_data_model/src/complex_isi.rs
Outdated
Show resolved
Hide resolved
cd45578
to
4656ecc
Compare
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
- remove `cant_filter_singular_query`: this is now much clearer, as the singular and iterable queries have different entrypoints, no need to test this anymore. - update `transparent_api_private_item`: use a different private item Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…w dsl, integrate into the client Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
This also fixes how an instruction failing to find a domain or an asset definition gets reported by the executor (by returning a FindError instead of a string) Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…ndXXXMetadata` Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…aw_filter` -> `filter` Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…p some others Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
- `PredicateTrait` -> `EvaluatePredicate` - `IterableQueryIterator` -> `QueryIterator` - `IterableQueryBuilder` -> `QueryBuilder` Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…the queries This renames prometheus metrics and `FindAllParameters` variant of `SingularQueryBox` enum Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…ension trait Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…onna be the "default" Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
- `ClientQueryCursor` and `SmartContractQueryCursor` -> `QueryCursor` - `ClientQueryError` -> `QueryError` - `QueryRequest::StartIterable` -> `QueryRequest::Start` - `QueryRequest::ContinueIterable` -> `QueryRequest::Continue` - `Pagination`'s `start` field -> `offset` Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…tQueryRequestHead::assemble` private Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…g error in executor Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
…from public API Signed-off-by: ⭐️NINIKA⭐️ <dcnick3@users.noreply.github.com>
7a86f22
to
6ff772d
Compare
Description
This PR introduces a lot of changes to the query subsystem. I tried to make the changes gradually in smaller PRs, but it ended up being very intertwined =(
Summary of changes
PredicateBox
there is a predicate type for each queryable type (likeAccountPredicateBox
), withCompountPredicate<T>
providing the logical operations on top.Iterator::filter
closure (|obj| obj.field.inner_field.starts_with("hello")
) and is inspired by diesel's DSL.QueryExecutor
traitThe primary key question
Currently we have a bunch of queries that give you an object using its Primary Key (like
FindAccountById
). They can be implemented by filtering a basis query that returns all objects of such type (likeFindAllAccounts
). However, with naïve implementation (which this PR currently does) this would result inO(n)
complexity instead ofO(log n)
that we currently have.There is a way to do these lookups fast: just pattern-match the filter on the shape
|obj| obj.id.eq(expected_id)
and use the primary key index instead of linear scan. We can even use a similar approach to speed up other types of queries, if deemed necessary.However, merging it as-is would mean that the primary queries would be slow. I can probably add the PK queries back for now, if that's unacceptable.
Future work
These are improvements I see that can be made to this PR, but I would prefer them to be implemented in separate PRs. I'll convent them to issues when this PR gets closer to getting merged.
Perf:
Ergonomics:
client::account::all()
et al, as this API doesn't provide much value compared toFindAccounts::new()
and doesn't exist in smart contracts (revise query API on client #4892)client::domain::by_id
, with them still relying on filtered queriesimpl Into<String>
for ids (Make methods accepting ids by value generic over its referencedness #4832)Misc:
Display
representations for queries & filters (right now printing them withDebug
)Linked issue
Closes #4569, #4720, #3720, #3671.
Checklist
I still have a lot of doc strings, an overview of the predicate DSL implementation and some tests to write, hence marking this PR as a draft.
#[warn(missing_docs)]
SignedQueryCandidate