Offset fails with multiple order statements

[Awoogamuffin]

• What version of Dgraph are you using?

1.0.14 (latest at time of writing)

• Have you tried reproducing the issue with latest release

Yes

• Steps to reproduce the issue (command/config used to run

If you order a set of results with two ordercommands (for example “orderdesc: firstName, orderdesc: lastName) then use offset, the results are not as expected.

From multiple tests it is clear that offset discards the first N results not of the final order (using both firstName and lastName), but only the first N results of what you would get if you only used the first order command.

• Expected behaviour and actual result.

For multiple examples of datasets (using both datetime values and string values) displaying this behaviour go to this thread in the discuss forums:

https://discuss.dgraph.io/t/offset-appears-not-to-work-with-multiple-order-statements/4421/13

[pawanrawal]

I can look into this.

[pawanrawal]

This is a bug. While doing the first sort, we need to consider all the uids in the page (having the same predicate value) where the offset falls, for subsequent sorts. I am working on a fix. The fix needs to happen both for sortWithoutIndex and sortWithIndex.

[pawanrawal]

This should be fixed with a799030. Not sure what else has to be done before closing this issue. @danielmai do we also cherry-pick the commit onto the release/v1 branch?

[danielmai]

@danielmai do we also cherry-pick the commit onto the release/v1 branch?

Any bug fixes should be cherry-picked into release/v1.0 to get into the v1.0.x series of releases.

[pawanrawal]

Thanks, I cherry-picked the commit into release/v1.0 but the CI doesn't seem to be happy about it.

https://teamcity.dgraph.io/viewLog.html?buildId=12506&buildTypeId=Dgraph_Ci, any ideas to what could be wrong here? I didn't change the test.sh script as part of my change.

[Awoogamuffin]

Thanks for looking into it!

[martinmr]

@pawanrawal: I have reverted your change for now. Cherry-picks into release/v1.0 are supposed to follow the same process as changes to master (open a PR but set the target branch to release/v1.0).

There have been a lot of changes in master not in release/v1.0 so cherry-picking changes is not guaranteed to work. The current error is due to some changes in the test script that were not cherry-picked to release/v1.0. I'll try to fix it but that won't fix the tests. The tests in the query package were refactored so they won't work out of the gate. The fix shouldn't be to difficult as it is mostly renaming the methods that process the queries to their old names (i.e processQueryNoErr used to have a different name).

[pawanrawal]

Thanks for looking into it @martinmr. I'll send a PR against release/v1.0.

[danielmai]

This has been fixed in master with #3400 and in release/v1.0 with #3455. Thanks @pawanrawal!