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

bad sorting when using balance:asc #2330

Closed
corsaro1 opened this Issue Aug 23, 2018 · 8 comments

Comments

Projects
9 participants
@corsaro1

corsaro1 commented Aug 23, 2018

Expected behavior

When retrieving voters for a delegate i'd expect the sort field to be applied properly so that low balance accounts are returned first and page 2 contains voters that have a higher balance than all entries in page 1 (when using balance:asc).

Actual behavior

Instead voters are queried without any apparent ordering and sorting is applied after the query has run

Steps to reproduce

please take a look at: https://github.com/LiskHQ/lisk/blob/development/modules/voters.js#L245-L252

the following 2 queries are just to see the problem:
/api/voters?sort=balance:asc&address=17670127987160191762L&limit=100&offset=0

/api/voters?sort=balance:asc&address=17670127987160191762L&limit=100&offset=100

Which version(s) does this affect? (Environment, OS, etc...)

core 1

@MaciejBaj MaciejBaj added this to the Version 1.3.0 milestone Aug 23, 2018

@MaciejBaj MaciejBaj added this to To do in Version 1.3.0 via automation Aug 23, 2018

@MaciejBaj

This comment has been minimized.

Show comment
Hide comment
@MaciejBaj

MaciejBaj Aug 23, 2018

Member

Thanks, @corsaro1, we should move sort to the corresponding DB query, so the records are ordered in an expected way.

Member

MaciejBaj commented Aug 23, 2018

Thanks, @corsaro1, we should move sort to the corresponding DB query, so the records are ordered in an expected way.

@diego-G diego-G added this to Sprint Backlog in Lisk Pipelines Aug 24, 2018

@yatki

This comment has been minimized.

Show comment
Hide comment
@yatki

yatki Aug 28, 2018

Member

I think having sort functionality here makes sense (with balance).

We can basically extend get_voters.sql to be like below:

SELECT t2.address, t2.balance, encode(t2."publicKey", 'hex') AS "publicKey" FROM mem_accounts2delegates t1
INNER JOIN mem_accounts t2 ON t1."accountId" = t2.address
WHERE t1."dependentId" = ${publicKey}
ORDER BY "${sortField:raw}" ${sortMethod:raw}
LIMIT ${limit} OFFSET ${offset}

And then we can get rid of populatedVoters task in modules/voters.js.

getVoters function is used only in voters controller, so it should be safe to update? There are no unit tests written for it yet. So I will need to write unit tests and update some integration tests.

@nazarhussain I think it doesn't make sense to sort voters with fields other than balance. username is also not possible, since it can be empty. What do you think?

Member

yatki commented Aug 28, 2018

I think having sort functionality here makes sense (with balance).

We can basically extend get_voters.sql to be like below:

SELECT t2.address, t2.balance, encode(t2."publicKey", 'hex') AS "publicKey" FROM mem_accounts2delegates t1
INNER JOIN mem_accounts t2 ON t1."accountId" = t2.address
WHERE t1."dependentId" = ${publicKey}
ORDER BY "${sortField:raw}" ${sortMethod:raw}
LIMIT ${limit} OFFSET ${offset}

And then we can get rid of populatedVoters task in modules/voters.js.

getVoters function is used only in voters controller, so it should be safe to update? There are no unit tests written for it yet. So I will need to write unit tests and update some integration tests.

@nazarhussain I think it doesn't make sense to sort voters with fields other than balance. username is also not possible, since it can be empty. What do you think?

@nazarhussain

This comment has been minimized.

Show comment
Hide comment
@nazarhussain

nazarhussain Aug 28, 2018

Contributor

@yatki Sorting on other field (publicKey) was a thought for determinist pagination, in case we remove sorting. So the solution you provided is perfect and good to go, so we keep the sorting for voters as well.

Contributor

nazarhussain commented Aug 28, 2018

@yatki Sorting on other field (publicKey) was a thought for determinist pagination, in case we remove sorting. So the solution you provided is perfect and good to go, so we keep the sorting for voters as well.

@vekexasia

This comment has been minimized.

Show comment
Hide comment
@vekexasia

vekexasia Sep 1, 2018

It's worth noticing that because no standard ordering is applied to the SQL the pagination is not really meaningful as postgres might decide to return some overlapping set of items for another page.

This is especially true if you use a pool of nodes trying to minimize the number of requests made to a single endpoint.

Pool operators might struggle a lot to fetch the his voters.

vekexasia commented Sep 1, 2018

It's worth noticing that because no standard ordering is applied to the SQL the pagination is not really meaningful as postgres might decide to return some overlapping set of items for another page.

This is especially true if you use a pool of nodes trying to minimize the number of requests made to a single endpoint.

Pool operators might struggle a lot to fetch the his voters.

@corsaro1

This comment has been minimized.

Show comment
Hide comment
@corsaro1

corsaro1 Sep 1, 2018

yes, it looks like this issue affects too pools voters list. At every query voters list is different from the precedent query and from the successive query. I am testing using this python code coming from dakk's lisk pool https://github.com/dakk/lisk-pool/blob/master/liskpool.py.

votes = requests.get (conf['node'] + '/api/voters?address=' + conf['address']).json ()['data']['votes']
		d = []

		for offset in range(0,votes,100):
			dpart = requests.get (conf['node'] + '/api/voters?address=' + conf['address'] + '&offset='+str(offset)+'&limit=100').json ()['data']['voters']
			d += dpart
		d = { "accounts": d }

corsaro1 commented Sep 1, 2018

yes, it looks like this issue affects too pools voters list. At every query voters list is different from the precedent query and from the successive query. I am testing using this python code coming from dakk's lisk pool https://github.com/dakk/lisk-pool/blob/master/liskpool.py.

votes = requests.get (conf['node'] + '/api/voters?address=' + conf['address']).json ()['data']['votes']
		d = []

		for offset in range(0,votes,100):
			dpart = requests.get (conf['node'] + '/api/voters?address=' + conf['address'] + '&offset='+str(offset)+'&limit=100').json ()['data']['voters']
			d += dpart
		d = { "accounts": d }
@biolypl

This comment has been minimized.

Show comment
Hide comment
@biolypl

biolypl Sep 1, 2018

I confirm the issues with :desc sorting

biolypl commented Sep 1, 2018

I confirm the issues with :desc sorting

@karek314

This comment has been minimized.

Show comment
Hide comment
@karek314

karek314 Sep 1, 2018

Workaround - do not use sorting. Sort yourself.

karek314 commented Sep 1, 2018

Workaround - do not use sorting. Sort yourself.

@diego-G diego-G added this to New Issues in Version 1.1.0 via automation Sep 4, 2018

@diego-G diego-G removed this from To do in Version 1.3.0 Sep 4, 2018

@diego-G diego-G modified the milestones: Version 1.3.0, Version 1.1.0 Sep 4, 2018

@shuse2 shuse2 closed this in caf2462 Sep 4, 2018

Version 1.1.0 automation moved this from New Issues to Closed Issues Sep 4, 2018

@diego-G diego-G removed this from Sprint Backlog in Lisk Pipelines Sep 4, 2018

@4miners

This comment has been minimized.

Show comment
Hide comment
@4miners

4miners Sep 5, 2018

Member

Reopened. We decided to include the fix for this issue as part of 1.1.0 release but the change implemented is a breaking one (username parameter is removed from sort fields). We cannot do this as part of the minor release.

Member

4miners commented Sep 5, 2018

Reopened. We decided to include the fix for this issue as part of 1.1.0 release but the change implemented is a breaking one (username parameter is removed from sort fields). We cannot do this as part of the minor release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment