Skip to content
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

Merged
merged 36 commits into from
Aug 14, 2024
Merged

Conversation

DCNick3
Copy link
Contributor

@DCNick3 DCNick3 commented Jul 10, 2024

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

  • Queries are now split into two categories: iterable queries which return sequences of results and singular queries that return a single value. They are now fully disjoint traits stored in different Box enums, as they require very different handling.
  • Query filters (aka predicates) are now typed depending on the type of the iterable query they are applied to (Restrict types of predicates applied to queries depending on the query output type (type-safe queries) #4569). Instead of a single PredicateBox there is a predicate type for each queryable type (like AccountPredicateBox), with CompountPredicate<T> providing the logical operations on top.
  • A new rust DSL to construct the predicates is introduced. It is designed to look very close to how one would write an Iterator::filter closure (|obj| obj.field.inner_field.starts_with("hello")) and is inspired by diesel's DSL.
  • HTTP clients and wasm code (smart contracts, triggers and executor) are now using a common query builder struct, generic over a new QueryExecutor trait
  • The HTTP API is majorly revamped to pass all of the params as scale-encoded values. This simplifies the torii handler, makes it more type-safe (can't add pagination to singular query) and allows sharing more code with wasm
  • All of the queries that can be implemented as filtered versions of other queries are now removed. Here's a rough google table I made to keep track of all of them: https://docs.google.com/spreadsheets/d/1Cx9cU4wLk1ZyIa3Igx0NN9azl9dfqcXXMJMLKTMxes4/edit?usp=sharing

The 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 (like FindAllAccounts). However, with naïve implementation (which this PR currently does) this would result in O(n) complexity instead of O(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:

  • the Primary Key optimization (see above, Add optimizations for primary key queries #4895)
  • make queries truly lazy (put them into the query store without collecting them all in a vec); Needs investigation whether that'll actually be beneficial and some clever implementation (need to store an iterator with lifetime tied to iroha state snapshot).

Ergonomics:

Misc:

  • Display representations for queries & filters (right now printing them with Debug)
  • check that the iterable query continuation is executed by the same authority as the one that started it
  • get rid of more singular queries (Get rid of the remaining singular queries #4933)

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.

  • add more docs and remove all the #[warn(missing_docs)]
  • add more tests of the DSL
  • add a test checking the validation performed on SignedQueryCandidate
  • fix client cli tests
  • fix ui tests
  • make CI pass

@github-actions github-actions bot added the api-changes Changes in the API for client libraries label Jul 10, 2024
@DCNick3 DCNick3 changed the title Simplify query filters refactor: query filters Jul 10, 2024
@DCNick3 DCNick3 force-pushed the simplify-query-filters branch 2 times, most recently from 6d4f55f to bfb2f31 Compare July 10, 2024 17:32
@DCNick3 DCNick3 self-assigned this Jul 11, 2024
@Erigara Erigara self-assigned this Jul 12, 2024
@mversic mversic self-assigned this Jul 15, 2024
@Erigara
Copy link
Contributor

Erigara commented Jul 15, 2024

Are projections smt we can introduce later?
To get rid of queries like FindAllRoleIds into smt like query_iter(FindAllRoles).with_projection(|role| role.id)

@DCNick3
Copy link
Contributor Author

DCNick3 commented Jul 15, 2024

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.

@Erigara
Copy link
Contributor

Erigara commented Jul 15, 2024

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.

Degradation in PK quires might introduce overhead into permission checks in the executor, so we should be careful with it.
Imo we can return PK queries for now and than gradually replace them filters after optimization will be introduced.

@Erigara
Copy link
Contributor

Erigara commented Jul 15, 2024

Can you describe briefly describe idea behind new predicates?

Like i can see now that we have AstPredicate, AtomPredicate, ...

What are stages predicate go though?

Copy link
Contributor

@Erigara Erigara left a 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.

client/tests/integration/queries/query_errors.rs Outdated Show resolved Hide resolved
client/tests/integration/queries/role.rs Outdated Show resolved Hide resolved
client/tests/integration/sorting.rs Outdated Show resolved Hide resolved
core/src/smartcontracts/isi/account.rs Show resolved Hide resolved
core/src/smartcontracts/isi/query.rs Show resolved Hide resolved
core/src/smartcontracts/isi/query.rs Show resolved Hide resolved
core/src/smartcontracts/isi/query.rs Show resolved Hide resolved
data_model/src/lib.rs Show resolved Hide resolved
data_model/src/query/mod.rs Outdated Show resolved Hide resolved
data_model/src/query/predicate/mod.rs Outdated Show resolved Hide resolved
@DCNick3
Copy link
Contributor Author

DCNick3 commented Jul 16, 2024

@Erigara

Can you describe briefly describe idea behind new predicates?
Like i can see now that we have AstPredicate, AtomPredicate, ...
What are stages predicate go though?

I've pushed some docs, including explanations of this topic. You would probably want to take a look at query::predicate::{self, prototypes, projectors}. If I missed any of the topics or any of the explanations are bad - please let me know!

@mversic
Copy link
Contributor

mversic commented Jul 17, 2024

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

client/src/query.rs Show resolved Hide resolved
client/tests/integration/asset.rs Outdated Show resolved Hide resolved
client/tests/integration/asset.rs Outdated Show resolved Hide resolved
client/src/query.rs Outdated Show resolved Hide resolved
client/src/query.rs Outdated Show resolved Hide resolved
client/src/query.rs Show resolved Hide resolved
data_model/src/query/builder.rs Outdated Show resolved Hide resolved
smart_contract/src/lib.rs Outdated Show resolved Hide resolved
smart_contract/src/lib.rs Outdated Show resolved Hide resolved
@DCNick3
Copy link
Contributor Author

DCNick3 commented Jul 17, 2024

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?

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 FindExecutorDataModel for example, just don't make sense as iterable queries.

@DCNick3 DCNick3 force-pushed the simplify-query-filters branch 2 times, most recently from 3b0f875 to 5047a82 Compare July 18, 2024 08:04
client/tests/integration/pagination.rs Show resolved Hide resolved
client/tests/integration/queries/account.rs Show resolved Hide resolved
data_model/src/query/builder.rs Outdated Show resolved Hide resolved
data_model/src/query/builder.rs Outdated Show resolved Hide resolved
core/src/smartcontracts/isi/domain.rs Outdated Show resolved Hide resolved
data_model/src/query/mod.rs Outdated Show resolved Hide resolved
data_model/src/query/mod.rs Show resolved Hide resolved
data_model/src/query/mod.rs Outdated Show resolved Hide resolved
smart_contract/src/lib.rs Outdated Show resolved Hide resolved
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>
@mversic mversic merged commit edf2311 into hyperledger-iroha:main Aug 14, 2024
17 of 29 checks passed
@DCNick3 DCNick3 added the queries label Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-changes Changes in the API for client libraries config-changes Changes in configuration and start up of the Iroha queries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restrict types of predicates applied to queries depending on the query output type (type-safe queries)
5 participants