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

[db] Add values() to search() #1244

Closed
teodosin opened this issue Sep 11, 2024 · 3 comments · Fixed by #1284
Closed

[db] Add values() to search() #1244

teodosin opened this issue Sep 11, 2024 · 3 comments · Fixed by #1284
Assignees
Labels

Comments

@teodosin
Copy link

teodosin commented Sep 11, 2024

Hey again :) Back with another feature request.

I found it odd that a search() query can't return the key-value pairs from nodes or edges. It feels like a natural thing to do but is as of yet unavailable. Currently we have to do two queries to get data from a search. The first one would be to get the node and edge id's from the search() and the second one would be to use those id's to run a select() query to get the actual data.

// Currently have to do this
let elems = db.exec(&QueryBuilder::search.from("root").query;

let ids: Vec<DbId> = links.unwrap().elements.iter().map(|elem| {
    elem.id
}).collect();

let full = self.db.exec(&QueryBuilder::select().values(vec![]).ids(ids).query());


// But why not just this?
let full = self.db.exec(&QueryBuilder::search().values(vec![]).ids(ids).query());

I would presume that having to run two queries to essentially accomplish one thing would have an effect on performance, even if marginal. And of course this would be more ergonomic. If there is a technical reason why this couldn't be done, perhaps you could mention it on the queries doc page.

@michaelvlach
Copy link
Collaborator

michaelvlach commented Sep 11, 2024

Hi, technically this will always be two queries. One where you search the graph and the other that fetches the associated data. I agree that some sort of syntactic sugar that allows them to be combined into one would be nice. There already is but perhaps not ideal because you could already achieve what you want quite easily as you can put search queries wherever you would put ids. Your example would be idiomatically written as:

let full = self.db.exec(&QueryBuilder::select().ids(QueryBuilder::search.from("root").query()).query());

If you wanted only some of the values you would throwin the .values() after select(). And doing it this way is a little bit more efficient than chaining the queries as in your original example.

EDIT: Also you can feed QueryResult wherever you would put ids as well and there is the QueryResult::ids() function that you can use to get the ids (no need to iterate and collect manually). So it can be quite convenient already but does not look like a single query.


That being said let's discuss the potential short-hands or "sugars" to make this even nicer. First of all the following are always equivalent to the select+search query. Those are the two fundamental operations the database do regardless how the query appears. I will mention it again in the end.

What I am long considering:

let full = self.db.exec(&QueryBuilder::select().from("root").where_().node().query());

One of the oldest variants, even predates this repository and was implemented in the original C++ prototype. I have ultimately rejected it because it is confusing or rather ambivalent. The from() implies search semantically while in English it could naturally mean an identifier to be selected from, i.e. node alias here. To alleviate that one would need to do QueryBuilder::select().search() or select_search. I did not like any of those and it is not much better than current combined queries as shown above.

The variant you proposed allowing values in search directly:

let full = self.db.exec(&QueryBuilder::search().values(vec![]).from("root").where_().node().query()

This looks nice and I am leaning towards implementing it. However it breaks some fundamentals of the current query system. Following the Rust explicitness I wanted the query system to always be transparent. Having a convenience alias for a particular builder step such as elements<T>() for values(T::db_keys() is a different to having a "morphing" query. What I mean is this:

QueryBuilder::search() //this is a search query
QueryBuilder::search().values() //this is a select query with nested search query

That does not seem very transparent to me and is the primary reason why I have not implemented it yet. But I recognize that the current way of chained or nested (idiomatically) queries is not ideal either. Then again currently the search query can be fed in any other query (insert, remove, select, or even another search). With the above it would still be true but you would not be able to tell as easily what type the query is. Even though it starts as the search in the latter variant it is actually a select.

I ma happy to hear your thoughts on this! And I will definitely expand the documentation on the nested queries.

@michaelvlach michaelvlach self-assigned this Sep 11, 2024
@michaelvlach michaelvlach moved this to Todo in agdb Sep 11, 2024
@michaelvlach michaelvlach changed the title Add values() to search() [db] Add values() to search() Sep 11, 2024
@teodosin
Copy link
Author

Ah, I see. I totally forgot about nested queries. I'm fine with just that, honestly. Already much more convenient than doing the unwrapping in between like I did.

Regarding the convenience alias vs. morphing query, I think I get where you're coming from. From a usability perspective I'm not sure they'd be that different, since the current system already enforces a correct order to the functions which isn't entirely intuitive to me. I keep referencing the docs, existing code, and rust-analyzer. I don't mind it though. So maybe morphing queries wouldn't make this worse at all, and maybe there's something that could make the whole pattern more intuitive still. Seems like a challenging problem, to be sure.

Thanks for the thorough reply :)

@michaelvlach
Copy link
Collaborator

You are right in that users won't really care and the type system will enforce the correct query anyway (I even simulated it in the other APIs in Typescript and PHP so far that they only accept SearchQuery where appropriate and not any query). I think you convinced me so I will keep this open and implement the ability to morph seamlessly from search to select as I do agree it is very common and would be very convenient.

A note about the correct order of functions that was done deliberately for readability. :-) I did expect people to use their IDE and LSP like rust-analyzer (or Rust Rover) to get it right but overall point is that it is not a free-form and therefore when you go (or I go) look at othe people's queries they will look exactly the same. Each piece will always be in the same place. With enough of them written you will remember it anyway just like in SQL but it will not leave you wondering "what sorcery is this?" if someone does something weird which is not possible here (unlike SQL). The query system is directly influenced by my frustrations working with SQL (have you ever seen 1000+ lines SQL quey [yes, singular]?). I suppose it's always like this, Graydon created Rust from his frustrations with C++. So transparency, conciseness and "mostly one way to do things" were important to me when designing it. But then again ergonomics are also very important so I am not following some dogma with it, just being mindful of the original principles.

Cheers again for this issue, I would probably keep deliberating instead of finally doing it!

@michaelvlach michaelvlach moved this from Todo to In Progress in agdb Sep 18, 2024
michaelvlach added a commit that referenced this issue Sep 25, 2024
* wip

* first version

* refactoring

* select search

* keys & key_count

* add more alts

* add search to inserts

* Update search.rs

* update readme

* update examples

* update efficient agdb

* Update queries.en-US.md

* Update quickstart.en-US.mdx
@github-project-automation github-project-automation bot moved this from In Progress to Done in agdb Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants