feat(dql): add keyword "every" to DQL #8976

EnricoMi · 2023-08-25T06:05:53Z

This adds the keyword every to DQL, which picks equidistant nodes. This provides a deterministic sample of the result that is proportional in size. It can be combined with first, offset, and after. Pagination is applied before every.

This supersedes #8107

Docs PR: None

mangalaman93

Thank you for updating the PR. This looks good to me, I have just one minor comment. I am also thinking that we should add test where every and after are used together. Thanks

query/query.go

EnricoMi · 2023-08-25T10:01:46Z

we should add test where every and after are used together.

Added in https://github.com/dgraph-io/dgraph/pull/8976/files#diff-99902c080bca80ed1ad52da34ea4041069d1c64a52317fc17f2285e8170ae6ddR1085

mangalaman93

one minor comment, LGTM otherwise. Is it possible for you to raise a corresponding docs PR on https://github.com/dgraph-io/dgraph-docs

query/query.go

EnricoMi · 2023-08-25T11:22:19Z

I'd love to run tests locally, but the instructions in CONTRIBUTING.md seem outdated and following the instructions from ci-dgraph-tests.yml don't work either:

cd t; ./t
testutil: "" localhost:9080 localhost:5080
...
Running tests for 121 packages.
Sent 1/121 packages for processing.
Bringing up cluster test-013-1 for package: ../dgraph/docker-compose.yml ...
CLUSTER UP: test-013-1. Package: ../dgraph/docker-compose.yml
Health for test-013-1_alpha6_1 failed: Get "http://localhost:49272/health": read tcp 127.0.0.1:35362->127.0.0.1:49272: read: connection reset by peer. Response: "". Retrying...
Health for test-013-1_alpha6_1 failed: Get "http://localhost:49272/health": dial tcp 127.0.0.1:49272: connect: connection refused. Response: "". Retrying...
Health for test-013-1_alpha6_1 failed: Get "http://localhost:49272/health": dial tcp 127.0.0.1:49272: connect: connection refused. Response: "". Retrying...
...

Running dgraph version in the dgraph/dgraph:local image returns:

dgraph: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by dgraph)
dgraph: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by dgraph)

EnricoMi · 2023-08-25T11:25:50Z

I have reworked the notion of every in conjunction with first, after and offset. The every is applied before pagination, so that the result of every can be paginated, not the paginated result is every-ed.

This required to change the way every works. It now picks the last uid from every-sized batches, so that paginating with after works: 2e4f90f

EnricoMi · 2023-08-25T12:31:36Z

I have managed to get tests working by setting contrib/Dockerfile to ubuntu:22.04 to match my dev machine.

mangalaman93

Looks much better too. One question before it is ready to go.

query/query4_test.go

query/query.go

Signed-off-by: Enrico Minack <github@enrico.minack.dev>

query/query.go

mangalaman93 · 2023-08-28T05:06:15Z

@EnricoMi Could you look at the CI failures?

Also, I only have an incomplete version of this PR #8977. If you like, you could take over a complete it. It should be a very similar PR.

EnricoMi · 2023-08-28T13:09:31Z

@mangalaman93 happy to finish #8977 once I understand the notion of having multiple sg.uidMatrix.

…r sg.uidMatrix elements

EnricoMi · 2023-08-29T06:12:12Z

Tests should all pass now, will look into docs PR next.

mangalaman93 · 2023-08-29T12:32:07Z

@EnricoMi upgrade tests are failing but don't worry about those. They are failing because the older versions of Dgraph do not support "every" keyword. I will fix those.

harshil-goel · 2023-08-29T12:39:19Z

query/query.go

+	}
+
+	// Re-merge the UID matrix.
+	sg.DestUIDs = algo.MergeSorted(sg.uidMatrix)


Can you also write unit test for applyEvery function?
I think you need to first MergeSort the entire uidMatrix and then take every ith element? Right now I don't think we will get the correct result.

mangalaman93 · 2023-08-29T13:40:11Z

query/query4_test.go

@@ -988,6 +989,142 @@ func TestCascadeSubQuery1(t *testing.T) {
 	}`, js)
 }

+func TestHasEvery(t *testing.T) {
+	query := `{


add this line as the first line of the test in each new test that you have added. This should fix the upgrade tests. Thanks

dgraphtest.ShouldSkipTest(t, "071c85b59fdf80ade88f4571e9cf8c867fae1f6a", dc.GetVersion())

mangalaman93 · 2023-08-29T15:44:01Z

@EnricoMi one more question, what is the use case of "every" word? Why can't we just use "random"?

EnricoMi · 2023-08-30T05:53:03Z

@mangalaman93 the use case is to partition the result set (the entire graph) into equi-sized partitions with one query to get boundary uids that are then used with after to fetch results in parallel: https://discuss.dgraph.io/t/sample-your-result/9055/1 With the random key the size of partitions would not be as useful.

Using offset is not an option as it has O(N) time complexity, where after has constant time: https://discuss.dgraph.io/t/pagination-with-offset-should-scale-like-after/8577

Looking at the code, the every keyword seems of O(N) complexity as well. Maybe the sampling should happen remotely in worker.ProcessTaskOverNetwork to retrieve only the sample of uids (e.g. every 1,000,000th uid):

dgraph/query/query.go

Line 2160 in 890c1f7

result, err := worker.ProcessTaskOverNetwork(ctx, taskQuery)

mangalaman93 · 2023-08-30T18:07:17Z

Looking at the code, the every keyword seems of O(N) complexity as well. Maybe the sampling should happen remotely in worker.ProcessTaskOverNetwork to retrieve only the sample of uids (e.g. every 1,000,000th uid):

that does seem like an interesting idea. I just wonder whether it is feasible to do given that results may get filtered out later sometimes. If you want, you can look into it. Thanks for the explanation for adding 'every' to the query language.

EnricoMi · 2023-09-05T09:03:05Z

that does seem like an interesting idea

I may need some pointer to the code where that worker.ProcessTaskOverNetwork is being executed.

EnricoMi · 2023-09-26T15:32:33Z

@mangalaman93 ping

EnricoMi requested review from mangalaman93, meghalims, sanjayk-github-dev, harshil-goel, all-seeing-code, billprovince and joshua-goldstein as code owners August 25, 2023 06:05

dgraph-bot added area/testing Testing related issues area/querylang Issues related to the query language specification and implementation. area/core internal mechanisms go Pull requests that update Go code labels Aug 25, 2023

EnricoMi mentioned this pull request Aug 25, 2023

feat(DQL): Add keyword every to DQL #8107

Closed

mangalaman93 requested changes Aug 25, 2023

View reviewed changes

query/query.go Outdated Show resolved Hide resolved

mangalaman93 changed the title ~~feat(DQL): Add keyword every to DQL~~ feat(dql): add keyword "every" to DQL Aug 25, 2023

EnricoMi force-pushed the branch-add-every-to-dql-2 branch 2 times, most recently from f93b115 to b3a2e9f Compare August 25, 2023 10:00

mangalaman93 previously approved these changes Aug 25, 2023

View reviewed changes

query/query.go Outdated Show resolved Hide resolved

EnricoMi dismissed mangalaman93’s stale review via 52d436b August 25, 2023 11:22

EnricoMi force-pushed the branch-add-every-to-dql-2 branch from b3a2e9f to 52d436b Compare August 25, 2023 11:22

mangalaman93 requested changes Aug 25, 2023

View reviewed changes

query/query4_test.go Outdated Show resolved Hide resolved

EnricoMi commented Aug 25, 2023

View reviewed changes

query/query.go Show resolved Hide resolved

EnricoMi added 5 commits August 25, 2023 15:03

Add every to DQL

058bf1d

Signed-off-by: Enrico Minack <github@enrico.minack.dev>

Consider uids after last uid picked from uidList as skipped

f3b6448

Signed-off-by: Enrico Minack <github@enrico.minack.dev>

Limit scope of err

a9d1ea7

Signed-off-by: Enrico Minack <github@enrico.minack.dev>

Remove usage of codec and sroar

ddd4dd0

Signed-off-by: Enrico Minack <github@enrico.minack.dev>

Add test with after and every

e3fb03e

Signed-off-by: Enrico Minack <github@enrico.minack.dev>

EnricoMi added 2 commits August 25, 2023 15:03

Apply every before pagination

bf247a5

Signed-off-by: Enrico Minack <github@enrico.minack.dev>

Pick last uid in Every uids, not first

a8e9948

Signed-off-by: Enrico Minack <github@enrico.minack.dev>

EnricoMi force-pushed the branch-add-every-to-dql-2 branch from 52d436b to a8e9948 Compare August 25, 2023 13:05

Fix expectation for TestHasEveryWithFirstOffset

6a673bf

EnricoMi commented Aug 25, 2023

View reviewed changes

query/query.go Show resolved Hide resolved

harshil-goel previously approved these changes Aug 25, 2023

View reviewed changes

Revert evaluation before pagination, revert offset carry-over to othe…

890c1f7

…r sg.uidMatrix elements

EnricoMi dismissed harshil-goel’s stale review via 890c1f7 August 29, 2023 06:08

mangalaman93 approved these changes Aug 29, 2023

View reviewed changes

harshil-goel reviewed Aug 29, 2023

View reviewed changes

mangalaman93 reviewed Aug 29, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dql): add keyword "every" to DQL #8976

feat(dql): add keyword "every" to DQL #8976

EnricoMi commented Aug 25, 2023 •

edited

mangalaman93 left a comment

EnricoMi commented Aug 25, 2023

mangalaman93 left a comment

EnricoMi commented Aug 25, 2023

EnricoMi commented Aug 25, 2023 •

edited

EnricoMi commented Aug 25, 2023

mangalaman93 left a comment

mangalaman93 commented Aug 28, 2023

EnricoMi commented Aug 28, 2023

EnricoMi commented Aug 29, 2023

mangalaman93 commented Aug 29, 2023

harshil-goel Aug 29, 2023

mangalaman93 Aug 29, 2023

mangalaman93 commented Aug 29, 2023

EnricoMi commented Aug 30, 2023

mangalaman93 commented Aug 30, 2023

EnricoMi commented Sep 5, 2023

EnricoMi commented Sep 26, 2023

feat(dql): add keyword "every" to DQL #8976

Are you sure you want to change the base?

feat(dql): add keyword "every" to DQL #8976

Conversation

EnricoMi commented Aug 25, 2023 • edited

mangalaman93 left a comment

Choose a reason for hiding this comment

EnricoMi commented Aug 25, 2023

mangalaman93 left a comment

Choose a reason for hiding this comment

EnricoMi commented Aug 25, 2023

EnricoMi commented Aug 25, 2023 • edited

EnricoMi commented Aug 25, 2023

mangalaman93 left a comment

Choose a reason for hiding this comment

mangalaman93 commented Aug 28, 2023

EnricoMi commented Aug 28, 2023

EnricoMi commented Aug 29, 2023

mangalaman93 commented Aug 29, 2023

harshil-goel Aug 29, 2023

Choose a reason for hiding this comment

mangalaman93 Aug 29, 2023

Choose a reason for hiding this comment

mangalaman93 commented Aug 29, 2023

EnricoMi commented Aug 30, 2023

mangalaman93 commented Aug 30, 2023

EnricoMi commented Sep 5, 2023

EnricoMi commented Sep 26, 2023

EnricoMi commented Aug 25, 2023 •

edited

EnricoMi commented Aug 25, 2023 •

edited