feat(Query): Add random keyword in DQL #7693
Conversation
|
|
There was a problem hiding this comment.
@ahsanbarkati -- can you work on the comment I have.
Thanks @Samyak2 for the PR. This is a good and clean PR, nicely written. @ahsanbarkati would help fix up the filter case I'm talking about.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Samyak2)
query/query.go, line 2422 at r1 (raw file):
// TODO: error handling here func (sg *SubGraph) applyRandom(ctx context.Context) error { sg.updateUidMatrix()
This should be done in the same way as we do first: N. So, if we have a filter, then we need to apply the filter first, and then choose N random nodes.
There was a problem hiding this comment.
Awesome @Samyak2. I have a few comments. Also, can we please add some tests? Do let me know if you need some help.
query/query.go
Outdated
| @@ -2395,6 +2414,34 @@ func ProcessGraph(ctx context.Context, sg, parent *SubGraph, rch chan error) { | |||
| rch <- childErr | |||
| } | |||
|
|
|||
| // applies "random" to lists inside uidMatrix | |||
| // Params.Random number of nodes are selected | |||
| // duplicates are not removed i.e., random selection by replacement is done | |||
There was a problem hiding this comment.
We should not have duplicate results. I'd suggest having some map to keep track of what all we have already picked to randomly pick n unique results.
There was a problem hiding this comment.
Okay, I'll do this, along with the filters application.
There was a problem hiding this comment.
I don't think you need to do anything for filters. You are already doing the random selection after filter application and pagination.
There was a problem hiding this comment.
Oh okay. That explains why I couldn't figure out how to do it by looking at pagination xD.
There was a problem hiding this comment.
I have pushed a commit fixing these.
To remove duplicates, I had to:
- introduce a new struct
UidKeyto store the index of a UID in the matrix - calculate total number of nodes and check if required number of nodes is less than that. If it's not, return all the nodes.
Because of the condition if sg.Params.Random >= totalNodes at line 2440, when the required number of nodes (sg.Params.Random) is equal to the total number of nodes, all of the nodes are returned without any processing. I did this thinking of it as an optimization (because there will be many overlaps in random numbers when only a few nodes are left to be selected). But I just realized that this will make it so that the order is not randomized when sg.Params.Random == totalNodes. Should I fix this to do the random selection even when all the nodes have to be selected?
…e random_ To remove duplicates, I had to: - introduce a new struct `UidKey` to store the index of a UID in the matrix - calculate total number of nodes and check if required number of nodes is less than that. If it's not, return all the nodes. - one caveat is that the order is not randomized when the requested number is equal to total number of nodes.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @ahsanbarkati and @Samyak2)
query/query.go, line 2432 at r2 (raw file):
// calculate total number of nodes totalNodes := 0
Had an initial look and I have some comments.
- I don't think we have to worry about duplicates here because a uid list within a uid matrix doesn't have any duplicates. Yes there could be duplicate uid within two different uids lists but we dont have to worry about that.
- @ahsanbarkati we should add some tests for this feature and how it works at root and at a child level.
- I reckon we can simplify this to just truncate all the uid lists with random elements in place instead of creating a new list.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @ahsanbarkati, @pawanrawal, and @Samyak2)
query/query.go, line 2432 at r2 (raw file):
Previously, pawanrawal (Pawan Rawal) wrote…
Had an initial look and I have some comments.
- I don't think we have to worry about duplicates here because a uid list within a uid matrix doesn't have any duplicates. Yes there could be duplicate uid within two different uids lists but we dont have to worry about that.
- @ahsanbarkati we should add some tests for this feature and how it works at root and at a child level.
- I reckon we can simplify this to just truncate all the uid lists with random elements in place instead of creating a new list.
@pawanrawal In the current approach, the way random nodes are selected is by repeatedly choosing a random index. So, even if there is just one uid list in the uidMatrix and it has all unique values, we'll end up with duplicates. Is that something acceptable?
There was a problem hiding this comment.
I see there is one issue with your approach. Suppose the query is for k random nodes, then you are selecting k nodes out of the whole UidMatrix. This is not the right thing to do.
You need to pick k random uids for each UidList in the UidMatrix. To make it clear, let me give you an example:
Suppose you run this query:
{
q2(func: has(name), random:5) {
name
uid
friend(random: 2) {
name
}
}
}
In the first go, we fetch all the Uids having the name predicate. In this case the UidMatrix will contain only one UidList. In the next iteration, when we'll fetch stuff for the children, for each uid in the UidList of uids containing name predicate, we'll get another uidList (list of Uids containing friends for the given UID).
So, you'll need to apply this random selection on each list rather than on complete UIdMatrix.
Secondly, as suggested by @pawanrawal, within a UidList, in order to select k random Uids. We can select a window of length k with random start index. We know that this will reduce the randomness but since we are doing this selection on Uids, it should be fine.
We'd love to merge your changes once this issue is addressed.
Also, we'd need some tests. It'd be great if you add some tests otherwise I'll all tests after merging your PR.
Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @ahsanbarkati, @pawanrawal, and @Samyak2)
|
Are you still working on this PR @Samyak2? If not, then I can merge your changes and then I can do the fixes in next PR. |
|
Yes, I'm still working on it. Sorry, I couldn't do it in the last week because of the end sem workload from college 😄. |
|
Awesome! Thanks @Samyak2. |
the random uids are now selected from each uid list instead of the uid matrix. This simplifies the process.
|
@ahsanbarkati thank you for the explanation, it makes a lot more sense now. I have implemented your suggestions in the latest commit:
I will look at adding tests for these now. |
|
I had a few issues while running the tests:
Output of `./t`Output of `./t -s -p "query"`I tried rebasing on master, the error persists. |
|
Awesome @Samyak2. Thanks for the nice work. Your changes looks good to me. I am just concerned if there will be performance issues by using Regarding the test, it seems like your docker is unable to spin up the containers. Can you try running Other thing to look at is by running By the way, I see that the CI has run fine. |
|
I am merging this PR, so that we can do further development/testing of the feature. Feel free to raise another PR with the tests. :) |
Okay, I will try debugging this.
Thank you! 😄 |
|
@ahsanbarkati I see you have added the tests in #7758! Are there any other tests to add here? If not, I could try fixing the performance issues by not using Also, is there a timeline on when this feature will be released? |
MichelDiz
added
cherry-pick
v22.0.0
This change implements the `random` function argument. If an argument `random: k` is provided to a dql function then random k results will be returned. For example: `func(has(name), random:5)` will choose 5 random nodes having the name predicate.
Fixes: https://discuss.dgraph.io/t/pick-random-node/10815
Changes
random: <number of nodes>Issues/future work
applyRandomfunctionThis is my first PR here (and I'm new to Go), there will be a lot more issues I'm not aware of.
Example
I have only tested this on a basic example from Ratel. This will need a lot more testing.
Mutation
{ set { _:luke <name> "Luke Skywalker" . _:luke <dgraph.type> "Person" . _:leia <name> "Princess Leia" . _:leia <dgraph.type> "Person" . _:han <name> "Han Solo" . _:han <dgraph.type> "Person" . _:lucas <name> "George Lucas" . _:lucas <dgraph.type> "Person" . _:irvin <name> "Irvin Kernshner" . _:irvin <dgraph.type> "Person" . _:richard <name> "Richard Marquand" . _:richard <dgraph.type> "Person" . _:sw1 <name> "Star Wars: Episode IV - A New Hope" . _:sw1 <release_date> "1977-05-25" . _:sw1 <revenue> "775000000" . _:sw1 <running_time> "121" . _:sw1 <starring> _:luke . _:sw1 <starring> _:leia . _:sw1 <starring> _:han . _:sw1 <director> _:lucas . _:sw1 <dgraph.type> "Film" . _:sw2 <name> "Star Wars: Episode V - The Empire Strikes Back" . _:sw2 <release_date> "1980-05-21" . _:sw2 <revenue> "534000000" . _:sw2 <running_time> "124" . _:sw2 <starring> _:luke . _:sw2 <starring> _:leia . _:sw2 <starring> _:han . _:sw2 <director> _:irvin . _:sw2 <dgraph.type> "Film" . _:sw3 <name> "Star Wars: Episode VI - Return of the Jedi" . _:sw3 <release_date> "1983-05-25" . _:sw3 <revenue> "572000000" . _:sw3 <running_time> "131" . _:sw3 <starring> _:luke . _:sw3 <starring> _:leia . _:sw3 <starring> _:han . _:sw3 <director> _:richard . _:sw3 <dgraph.type> "Film" . _:st1 <name> "Star Trek: The Motion Picture" . _:st1 <release_date> "1979-12-07" . _:st1 <revenue> "139000000" . _:st1 <running_time> "132" . _:st1 <dgraph.type> "Film" . } }Query
{ q(func: type(Person), random: 5) { name uid } }Result
This change is