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

fix(sort): Only filter out nodes with positive offsets. (#8077) #8441

Merged
merged 9 commits into from
Dec 2, 2022

Conversation

joshua-goldstein
Copy link
Contributor

@joshua-goldstein joshua-goldstein commented Nov 19, 2022

Cherry pick from #8077.

Steps to reproduce:

make image-local
cd contrib/local-test && make up
make schema-dql
make load-data-dql-rdf
make query-dql
curl: (52) Empty reply from server
make: *** [query-dql] Error 52
<alpha will crash and restart>

with the following minimal schema / data / query (should be placed in contrib/local-test):

schema.dql:

type Person {
    name
    age
    friend
}

name: string @index(term)  .
age: int @index(int) .
friend: [uid] @count .

dql-data.rdf

{
  set {
    _:michael <name> "Michael" .
    _:michael <dgraph.type> "Person" .
    _:michael <age> "314" .
    _:michael <friend> _:alice .
    _:michael <friend> _:bob .
    _:michael <friend> _:charlie .

    _:alice <name> "Alice" .
    _:alice <dgraph.type> "Person" .
    _:alice <age> "1" .

    _:bob <name> "Bob" .
    _:bob <dgraph.type> "Person" .
    _:bob <age> "2" .

    _:charlie <name> "Charlie" .
    _:charlie <dgraph.type> "Person" .
  }
}

query.dql

{
  michael_friends_first(func: allofterms(name, "Michael")) {
    name
    age
    friend (orderasc: age, offset: -1) {
      name
      age
    }
  }
}

Remarks

  • The cherry-pick does resolve the above crash
  • This issue exists in main
  • Why do we allow negative offsets? If they are not useful, we should return an error when a user attempt to query using a negative offset.

@joshua-goldstein joshua-goldstein added the cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release label Nov 19, 2022
@CLAassistant
Copy link

CLAassistant commented Nov 19, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ joshua-goldstein
❌ danielmai
You have signed the CLA already but the status is still pending? Let us recheck it.

@coveralls
Copy link

coveralls commented Nov 19, 2022

Coverage Status

Coverage decreased (-0.001%) to 37.181% when pulling fae2ad4 on cherry-pick-8077 into 7d7c02f on main.

@joshua-goldstein joshua-goldstein self-assigned this Nov 21, 2022
@mangalaman93 mangalaman93 self-requested a review November 26, 2022 19:12
Negative offsets (e.g., offset: -4) can cause panics when sorting. This can happen when the query has the following characteristics:

1. The query is sorting on an indexed predicate
2. The results include nodes that also don't have the sorted predicate
3. A negative offset is used.

    (panic trace is from v20.11.2-rc1-23-gaf5030a5)
    panic: runtime error: slice bounds out of range [-4:]
    goroutine 1762633 [running]:
    github.com/dgraph-io/dgraph/worker.sortWithIndex(0x1fb12e0, 0xc00906a880, 0xc009068660, 0x0)
            /ext-go/1/src/github.com/dgraph-io/dgraph/worker/sort.go:330 +0x244d
    github.com/dgraph-io/dgraph/worker.processSort.func2(0x1fb12e0, 0xc00906a880, 0xc009068660, 0xc0090686c0)
            /ext-go/1/src/github.com/dgraph-io/dgraph/worker/sort.go:515 +0x3f
    created by github.com/dgraph-io/dgraph/worker.processSort
            /ext-go/1/src/github.com/dgraph-io/dgraph/worker/sort.go:514 +0x52a

(cherry picked from commit 74d833c)
@@ -546,6 +546,20 @@ func TestCascadeWithSort(t *testing.T) {
require.JSONEq(t, `{"data":{"me":[{"name": "Daryl Dixon","alive": false},{"name": "Rick Grimes","alive": true}]}}`, js)
}

// Resolved in PR #8441
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other tests in the past have given a full link to the issue, i.e.

// Regression test for https://github.com/dgraph-io/dgraph/issues/3657.

Copy link
Contributor

@meghalims meghalims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this Joshua. Please go ahead and merge

@all-seeing-code all-seeing-code merged commit 19febad into main Dec 2, 2022
@all-seeing-code all-seeing-code deleted the cherry-pick-8077 branch December 2, 2022 14:33
@damonfeldman
Copy link

Note this fixes errors reported as panic: runtime error: slice bounds out of range [-4:] where a negative offset is used, a predicate is indexed, and nodes are included that do not have the predicate. It is questionable why or when negative offsets are useful, but this can happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release
Development

Successfully merging this pull request may close these issues.

9 participants