From b353bad9f14c34273ca28cd51c0e943b40554229 Mon Sep 17 00:00:00 2001 From: Graylin Kim Date: Tue, 21 May 2019 16:19:17 -0400 Subject: [PATCH 1/4] Use withscores to optimize leaders call where rank can be inferred. --- leaderboard/leaderboard.py | 68 +++++++++++++------ .../competition_ranking_leaderboard_test.py | 2 +- ...se_competition_ranking_leaderboard_test.py | 2 +- .../reverse_tie_ranking_leaderboard_test.py | 2 +- .../tie_ranking_leaderboard_test.py | 2 +- 5 files changed, 50 insertions(+), 26 deletions(-) diff --git a/leaderboard/leaderboard.py b/leaderboard/leaderboard.py index 6ff042f..7a5043c 100644 --- a/leaderboard/leaderboard.py +++ b/leaderboard/leaderboard.py @@ -778,7 +778,7 @@ def leaders(self, current_page, **options): ''' return self.leaders_in(self.leaderboard_name, current_page, **options) - def leaders_in(self, leaderboard_name, current_page, **options): + def leaders_in(self, leaderboard_name, current_page, members_only=False, **options): ''' Retrieve a page of leaders from the named leaderboard. @@ -800,14 +800,33 @@ def leaders_in(self, leaderboard_name, current_page, **options): ending_offset = (starting_offset + page_size) - 1 - raw_leader_data = self._range_method( + response = self._range_method( self.redis_connection, leaderboard_name, int(starting_offset), int(ending_offset), - withscores=False) - return self._parse_raw_members( - leaderboard_name, raw_leader_data, **options) + withscores=not members_only) + + if members_only: + return [{self.MEMBER_KEY: member} for member in response] + + members = [] + ranks_for_members = [] + for i, (member, score) in enumerate(response): + members.append(member) + ranks_for_members.append({ + self.MEMBER_KEY: member, + self.RANK_KEY: starting_offset + i + 1, + self.SCORE_KEY: score, + }) + + if options.get('with_member_data', False): + self._with_member_data(leaderboard_name, members, ranks_for_members) + + if 'sort_by' in options: + self._sort_by(ranks_for_members, options['sort_by']) + + return ranks_for_members def all_leaders(self, **options): ''' @@ -1072,28 +1091,33 @@ def ranked_in_list_in(self, leaderboard_name, members, **options): ranks_for_members.append(data) - if ('with_member_data' in options) and (True == options['with_member_data']): - for index, member_data in enumerate(self.members_data_for_in(leaderboard_name, members)): - try: - ranks_for_members[index][self.MEMBER_DATA_KEY] = member_data - except: - pass + if options.get('with_member_data', False): + self._with_member_data(leaderboard_name, members, ranks_for_members) if 'sort_by' in options: - sort_value_if_none = float('-inf') if self.order == self.ASC else float('+inf') - if self.RANK_KEY == options['sort_by']: - ranks_for_members = sorted( - ranks_for_members, - key=lambda member: member.get(self.RANK_KEY) if member.get(self.RANK_KEY) is not None else sort_value_if_none - ) - elif self.SCORE_KEY == options['sort_by']: - ranks_for_members = sorted( - ranks_for_members, - key=lambda member: member.get(self.SCORE_KEY) if member.get(self.SCORE_KEY) is not None else sort_value_if_none - ) + self._sort_by(ranks_for_members, options['sort_by']) return ranks_for_members + def _with_member_data(self, leaderboard_name, members, ranks_for_members): + for index, member_data in enumerate(self.members_data_for_in(leaderboard_name, members)): + try: + ranks_for_members[index][self.MEMBER_DATA_KEY] = member_data + except: + pass + + def _sort_by(self, ranks_for_members, sort_by): + sort_value_if_none = float('-inf') if self.order == self.ASC else float('+inf') + if self.RANK_KEY == sort_by: + ranks_for_members.sort( + key=lambda member: member.get(self.RANK_KEY) if member.get(self.RANK_KEY) is not None else sort_value_if_none + ) + elif self.SCORE_KEY == sort_by: + ranks_for_members.sort( + key=lambda member: member.get(self.SCORE_KEY) if member.get(self.SCORE_KEY) is not None else sort_value_if_none + ) + return ranks_for_members + def merge_leaderboards(self, destination, keys, aggregate='SUM'): ''' Merge leaderboards given by keys with this leaderboard into a named destination leaderboard. diff --git a/test/leaderboard/competition_ranking_leaderboard_test.py b/test/leaderboard/competition_ranking_leaderboard_test.py index 65bfd46..befdbcc 100644 --- a/test/leaderboard/competition_ranking_leaderboard_test.py +++ b/test/leaderboard/competition_ranking_leaderboard_test.py @@ -8,7 +8,7 @@ class CompetitionRankingLeaderboardTest(unittest.TestCase): def setUp(self): - self.leaderboard = CompetitionRankingLeaderboard('ties') + self.leaderboard = CompetitionRankingLeaderboard('ties', decode_responses=True) def tearDown(self): self.leaderboard.redis_connection.flushdb() diff --git a/test/leaderboard/reverse_competition_ranking_leaderboard_test.py b/test/leaderboard/reverse_competition_ranking_leaderboard_test.py index 483ae00..aaab401 100644 --- a/test/leaderboard/reverse_competition_ranking_leaderboard_test.py +++ b/test/leaderboard/reverse_competition_ranking_leaderboard_test.py @@ -10,7 +10,7 @@ class ReverseCompetitionRankingLeaderboardTest(unittest.TestCase): def setUp(self): self.leaderboard = CompetitionRankingLeaderboard( - 'ties', order=Leaderboard.ASC) + 'ties', order=Leaderboard.ASC, decode_responses=True) def tearDown(self): self.leaderboard.redis_connection.flushdb() diff --git a/test/leaderboard/reverse_tie_ranking_leaderboard_test.py b/test/leaderboard/reverse_tie_ranking_leaderboard_test.py index c9d40c4..4415ade 100644 --- a/test/leaderboard/reverse_tie_ranking_leaderboard_test.py +++ b/test/leaderboard/reverse_tie_ranking_leaderboard_test.py @@ -9,7 +9,7 @@ class ReverseTieRankingLeaderboardTest(unittest.TestCase): def setUp(self): - self.leaderboard = TieRankingLeaderboard('ties', order=Leaderboard.ASC) + self.leaderboard = TieRankingLeaderboard('ties', order=Leaderboard.ASC, decode_responses=True) def tearDown(self): self.leaderboard.redis_connection.flushdb() diff --git a/test/leaderboard/tie_ranking_leaderboard_test.py b/test/leaderboard/tie_ranking_leaderboard_test.py index a75f69a..3c12b8c 100644 --- a/test/leaderboard/tie_ranking_leaderboard_test.py +++ b/test/leaderboard/tie_ranking_leaderboard_test.py @@ -8,7 +8,7 @@ class TieRankingLeaderboardTest(unittest.TestCase): def setUp(self): - self.leaderboard = TieRankingLeaderboard('ties') + self.leaderboard = TieRankingLeaderboard('ties', decode_responses=True) def tearDown(self): self.leaderboard.redis_connection.flushdb() From d56dab4acb9a47a551891f8e5e8b4b8b99364db7 Mon Sep 17 00:00:00 2001 From: Graylin Kim Date: Wed, 22 May 2019 13:05:56 -0400 Subject: [PATCH 2/4] Leaderboard specific implementations. --- .../competition_ranking_leaderboard.py | 79 ++++++++++----- leaderboard/leaderboard.py | 99 +++++++++---------- leaderboard/tie_ranking_leaderboard.py | 65 ++++++++---- 3 files changed, 149 insertions(+), 94 deletions(-) diff --git a/leaderboard/competition_ranking_leaderboard.py b/leaderboard/competition_ranking_leaderboard.py index a249d6b..207f645 100644 --- a/leaderboard/competition_ranking_leaderboard.py +++ b/leaderboard/competition_ranking_leaderboard.py @@ -1,6 +1,4 @@ from .leaderboard import Leaderboard -from redis import StrictRedis, Redis, ConnectionPool -import math class CompetitionRankingLeaderboard(Leaderboard): @@ -78,22 +76,15 @@ def ranked_in_list_in(self, leaderboard_name, members, **options): scores = [] pipeline = self.redis_connection.pipeline() - for member in members: - if self.order == self.ASC: - pipeline.zrank(leaderboard_name, member) - else: - pipeline.zrevrank(leaderboard_name, member) - pipeline.zscore(leaderboard_name, member) - responses = pipeline.execute() for index, member in enumerate(members): data = {} data[self.MEMBER_KEY] = member - score = responses[index * 2 + 1] + score = responses[index] if score is not None: score = float(score) else: @@ -108,21 +99,11 @@ def ranked_in_list_in(self, leaderboard_name, members, **options): for index, rank in enumerate(self.__rankings_for_members_having_scores_in(leaderboard_name, members, scores)): ranks_for_members[index][self.RANK_KEY] = rank - if ('with_member_data' in options) and (True == options['with_member_data']): - for index, member_data in enumerate(self.members_data_for_in(leaderboard_name, members)): - ranks_for_members[index][self.MEMBER_DATA_KEY] = member_data + if options.get('with_member_data', False): + self._with_member_data(leaderboard_name, members, ranks_for_members) if 'sort_by' in options: - if self.RANK_KEY == options['sort_by']: - ranks_for_members = sorted( - ranks_for_members, - key=lambda member: member[ - self.RANK_KEY]) - elif self.SCORE_KEY == options['sort_by']: - ranks_for_members = sorted( - ranks_for_members, - key=lambda member: member[ - self.SCORE_KEY]) + self._sort_by(ranks_for_members, options['sort_by']) return ranks_for_members @@ -150,3 +131,55 @@ def __rankings_for_members_having_scores_in(self, leaderboard_name, members, sco responses = pipeline.execute() return [self.__up_rank(response) for response in responses] + + def _members_from_rank_range_internal( + self, leaderboard_name, start_rank, end_rank, members_only=False, **options): + ''' + Format ordered members with score as efficiently as possible. + ''' + response = self._range_method( + self.redis_connection, + leaderboard_name, + start_rank, + end_rank, + withscores=not members_only) + + if members_only or not response: + return [{self.MEMBER_KEY: member} for member in response] + + # Find out where the current rank started using the first two ranks + current_rank = None + current_score = None + current_rank_start = 0 + for index, (member, score) in enumerate(response): + if current_score is None: + current_rank = self.rank_for_in(leaderboard_name, member) + current_score = score + elif score != current_score: + next_rank = self.rank_for_in(leaderboard_name, member) + current_rank_start = current_rank - next_rank + index + break + + members = [] + ranks_for_members = [] + for index, (member, score) in enumerate(response): + members.append(member) + if score != current_score: + current_rank += (index - current_rank_start) + current_rank_start = index + current_score = score + + member_entry = { + self.MEMBER_KEY: member, + self.RANK_KEY: current_rank, + self.SCORE_KEY: score, + } + ranks_for_members.append(member_entry) + + if options.get('with_member_data', False): + self._with_member_data(leaderboard_name, members, ranks_for_members) + + if 'sort_by' in options: + self._sort_by(ranks_for_members, options['sort_by']) + + return ranks_for_members diff --git a/leaderboard/leaderboard.py b/leaderboard/leaderboard.py index 7a5043c..938c219 100644 --- a/leaderboard/leaderboard.py +++ b/leaderboard/leaderboard.py @@ -32,7 +32,7 @@ class Leaderboard(object): RANK_KEY = 'rank' @classmethod - def pool(self, host, port, db, pools={}, **options): + def pool(cls, host, port, db, pools={}, **options): ''' Fetch a redis connection pool for the unique combination of host and port. Will create a new one if there isn't one already. @@ -74,7 +74,7 @@ def __init__(self, leaderboard_name, **options): self.DEFAULT_GLOBAL_MEMBER_DATA) self.order = self.options.pop('order', self.DESC).lower() - if not self.order in [self.ASC, self.DESC]: + if self.order not in [self.ASC, self.DESC]: raise ValueError( "%s is not one of [%s]" % (self.order, ",".join([self.ASC, self.DESC]))) @@ -778,7 +778,7 @@ def leaders(self, current_page, **options): ''' return self.leaders_in(self.leaderboard_name, current_page, **options) - def leaders_in(self, leaderboard_name, current_page, members_only=False, **options): + def leaders_in(self, leaderboard_name, current_page, **options): ''' Retrieve a page of leaders from the named leaderboard. @@ -800,33 +800,11 @@ def leaders_in(self, leaderboard_name, current_page, members_only=False, **optio ending_offset = (starting_offset + page_size) - 1 - response = self._range_method( - self.redis_connection, + return self._members_from_rank_range_internal( leaderboard_name, int(starting_offset), int(ending_offset), - withscores=not members_only) - - if members_only: - return [{self.MEMBER_KEY: member} for member in response] - - members = [] - ranks_for_members = [] - for i, (member, score) in enumerate(response): - members.append(member) - ranks_for_members.append({ - self.MEMBER_KEY: member, - self.RANK_KEY: starting_offset + i + 1, - self.SCORE_KEY: score, - }) - - if options.get('with_member_data', False): - self._with_member_data(leaderboard_name, members, ranks_for_members) - - if 'sort_by' in options: - self._sort_by(ranks_for_members, options['sort_by']) - - return ranks_for_members + **options) def all_leaders(self, **options): ''' @@ -845,10 +823,8 @@ def all_leaders_from(self, leaderboard_name, **options): @param options [Hash] Options to be used when retrieving the leaders from the named leaderboard. @return the named leaderboard. ''' - raw_leader_data = self._range_method( - self.redis_connection, leaderboard_name, 0, -1, withscores=False) - return self._parse_raw_members( - leaderboard_name, raw_leader_data, **options) + return self._members_from_rank_range_internal( + leaderboard_name, 0, -1, **options) def members_from_score_range( self, minimum_score, maximum_score, **options): @@ -919,22 +895,8 @@ def members_from_rank_range_in( if ending_rank > self.total_members_in(leaderboard_name): ending_rank = self.total_members_in(leaderboard_name) - 1 - raw_leader_data = [] - if self.order == self.DESC: - raw_leader_data = self.redis_connection.zrevrange( - leaderboard_name, - starting_rank, - ending_rank, - withscores=False) - else: - raw_leader_data = self.redis_connection.zrange( - leaderboard_name, - starting_rank, - ending_rank, - withscores=False) - - return self._parse_raw_members( - leaderboard_name, raw_leader_data, **options) + return self._members_from_rank_range_internal( + leaderboard_name, starting_rank, ending_rank, **options) def top(self, number, **options): ''' @@ -1031,14 +993,11 @@ def around_me_in(self, leaderboard_name, member, **options): ending_offset = (starting_offset + page_size) - 1 - raw_leader_data = self._range_method( - self.redis_connection, + return self._members_from_rank_range_internal( leaderboard_name, int(starting_offset), int(ending_offset), - withscores=False) - return self._parse_raw_members( - leaderboard_name, raw_leader_data, **options) + **options) def ranked_in_list(self, members, **options): ''' @@ -1177,3 +1136,39 @@ def _parse_raw_members( return self.ranked_in_list_in(leaderboard_name, members, **options) else: return [] + + def _members_from_rank_range_internal( + self, leaderboard_name, start_rank, end_rank, members_only=False, **options): + ''' + Format ordered members with score as efficiently as possible. + ''' + response = self._range_method( + self.redis_connection, + leaderboard_name, + start_rank, + end_rank, + withscores=not members_only) + + if members_only or not response: + return [{self.MEMBER_KEY: member} for member in response] + + current_rank = start_rank + members = [] + ranks_for_members = [] + for index, (member, score) in enumerate(response): + members.append(member) + current_rank += 1 + member_entry = { + self.MEMBER_KEY: member, + self.RANK_KEY: current_rank, + self.SCORE_KEY: score, + } + ranks_for_members.append(member_entry) + + if options.get('with_member_data', False): + self._with_member_data(leaderboard_name, members, ranks_for_members) + + if 'sort_by' in options: + self._sort_by(ranks_for_members, options['sort_by']) + + return ranks_for_members diff --git a/leaderboard/tie_ranking_leaderboard.py b/leaderboard/tie_ranking_leaderboard.py index 05d1f14..5e8becb 100644 --- a/leaderboard/tie_ranking_leaderboard.py +++ b/leaderboard/tie_ranking_leaderboard.py @@ -1,7 +1,6 @@ from .leaderboard import Leaderboard from .leaderboard import grouper -from redis import StrictRedis, Redis, ConnectionPool -import math +from redis import Redis class TieRankingLeaderboard(Leaderboard): @@ -120,7 +119,7 @@ def rank_member_across( @param member_data [String] Optional member data. ''' for leaderboard_name in leaderboards: - self.rank_member_in(leaderboard, member, score, member_data) + self.rank_member_in(leaderboard_name, member, score, member_data) def rank_members_in(self, leaderboard_name, members_and_scores): ''' @@ -271,24 +270,11 @@ def ranked_in_list_in(self, leaderboard_name, members, **options): ranks_for_members.append(data) - if ('with_member_data' in options) and (True == options['with_member_data']): - for index, member_data in enumerate(self.members_data_for_in(leaderboard_name, members)): - try: - ranks_for_members[index][self.MEMBER_DATA_KEY] = member_data - except: - pass + if options.get('with_member_data', False): + self._with_member_data(leaderboard_name, members, ranks_for_members) if 'sort_by' in options: - if self.RANK_KEY == options['sort_by']: - ranks_for_members = sorted( - ranks_for_members, - key=lambda member: member[ - self.RANK_KEY]) - elif self.SCORE_KEY == options['sort_by']: - ranks_for_members = sorted( - ranks_for_members, - key=lambda member: member[ - self.SCORE_KEY]) + self._sort_by(ranks_for_members, options['sort_by']) return ranks_for_members @@ -300,3 +286,44 @@ def _ties_leaderboard_key(self, leaderboard_name): @return a key in the form of +leaderboard_name:ties_namespace+ ''' return '%s:%s' % (leaderboard_name, self.ties_namespace) + + def _members_from_rank_range_internal( + self, leaderboard_name, start_rank, end_rank, members_only=False, **options): + ''' + Format ordered members with score as efficiently as possible. + ''' + response = self._range_method( + self.redis_connection, + leaderboard_name, + start_rank, + end_rank, + withscores=not members_only) + + if members_only or not response: + return [{self.MEMBER_KEY: member} for member in response] + + current_member, current_score = response[0] + current_rank = self.rank_for_in(leaderboard_name, current_member) + current_score = response[0][1] + members = [] + ranks_for_members = [] + for index, (member, score) in enumerate(response): + if score != current_score: + current_rank += 1 + current_score = score + + members.append(member) + member_entry = { + self.MEMBER_KEY: member, + self.RANK_KEY: current_rank, + self.SCORE_KEY: score, + } + ranks_for_members.append(member_entry) + + if options.get('with_member_data', False): + self._with_member_data(leaderboard_name, members, ranks_for_members) + + if 'sort_by' in options: + self._sort_by(ranks_for_members, options['sort_by']) + + return ranks_for_members From 6354405571ca00e8d0b72c463cd4a14a98ee785d Mon Sep 17 00:00:00 2001 From: Graylin Kim Date: Mon, 10 Jun 2019 14:11:04 -0400 Subject: [PATCH 3/4] Revert changes pulled out into separate patches. --- leaderboard/tie_ranking_leaderboard.py | 2 +- test/leaderboard/competition_ranking_leaderboard_test.py | 2 +- .../leaderboard/reverse_competition_ranking_leaderboard_test.py | 2 +- test/leaderboard/reverse_tie_ranking_leaderboard_test.py | 2 +- test/leaderboard/tie_ranking_leaderboard_test.py | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/leaderboard/tie_ranking_leaderboard.py b/leaderboard/tie_ranking_leaderboard.py index 5e8becb..313bf96 100644 --- a/leaderboard/tie_ranking_leaderboard.py +++ b/leaderboard/tie_ranking_leaderboard.py @@ -119,7 +119,7 @@ def rank_member_across( @param member_data [String] Optional member data. ''' for leaderboard_name in leaderboards: - self.rank_member_in(leaderboard_name, member, score, member_data) + self.rank_member_in(leaderboard, member, score, member_data) def rank_members_in(self, leaderboard_name, members_and_scores): ''' diff --git a/test/leaderboard/competition_ranking_leaderboard_test.py b/test/leaderboard/competition_ranking_leaderboard_test.py index befdbcc..65bfd46 100644 --- a/test/leaderboard/competition_ranking_leaderboard_test.py +++ b/test/leaderboard/competition_ranking_leaderboard_test.py @@ -8,7 +8,7 @@ class CompetitionRankingLeaderboardTest(unittest.TestCase): def setUp(self): - self.leaderboard = CompetitionRankingLeaderboard('ties', decode_responses=True) + self.leaderboard = CompetitionRankingLeaderboard('ties') def tearDown(self): self.leaderboard.redis_connection.flushdb() diff --git a/test/leaderboard/reverse_competition_ranking_leaderboard_test.py b/test/leaderboard/reverse_competition_ranking_leaderboard_test.py index aaab401..483ae00 100644 --- a/test/leaderboard/reverse_competition_ranking_leaderboard_test.py +++ b/test/leaderboard/reverse_competition_ranking_leaderboard_test.py @@ -10,7 +10,7 @@ class ReverseCompetitionRankingLeaderboardTest(unittest.TestCase): def setUp(self): self.leaderboard = CompetitionRankingLeaderboard( - 'ties', order=Leaderboard.ASC, decode_responses=True) + 'ties', order=Leaderboard.ASC) def tearDown(self): self.leaderboard.redis_connection.flushdb() diff --git a/test/leaderboard/reverse_tie_ranking_leaderboard_test.py b/test/leaderboard/reverse_tie_ranking_leaderboard_test.py index 4415ade..c9d40c4 100644 --- a/test/leaderboard/reverse_tie_ranking_leaderboard_test.py +++ b/test/leaderboard/reverse_tie_ranking_leaderboard_test.py @@ -9,7 +9,7 @@ class ReverseTieRankingLeaderboardTest(unittest.TestCase): def setUp(self): - self.leaderboard = TieRankingLeaderboard('ties', order=Leaderboard.ASC, decode_responses=True) + self.leaderboard = TieRankingLeaderboard('ties', order=Leaderboard.ASC) def tearDown(self): self.leaderboard.redis_connection.flushdb() diff --git a/test/leaderboard/tie_ranking_leaderboard_test.py b/test/leaderboard/tie_ranking_leaderboard_test.py index 3c12b8c..a75f69a 100644 --- a/test/leaderboard/tie_ranking_leaderboard_test.py +++ b/test/leaderboard/tie_ranking_leaderboard_test.py @@ -8,7 +8,7 @@ class TieRankingLeaderboardTest(unittest.TestCase): def setUp(self): - self.leaderboard = TieRankingLeaderboard('ties', decode_responses=True) + self.leaderboard = TieRankingLeaderboard('ties') def tearDown(self): self.leaderboard.redis_connection.flushdb() From 452a214c0c86cd11bca2db153f08cffaa97bff3f Mon Sep 17 00:00:00 2001 From: Graylin Kim Date: Mon, 10 Jun 2019 14:20:14 -0400 Subject: [PATCH 4/4] Improve variable naming for clarity. --- leaderboard/tie_ranking_leaderboard.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/leaderboard/tie_ranking_leaderboard.py b/leaderboard/tie_ranking_leaderboard.py index 313bf96..5a8c0ed 100644 --- a/leaderboard/tie_ranking_leaderboard.py +++ b/leaderboard/tie_ranking_leaderboard.py @@ -302,21 +302,21 @@ def _members_from_rank_range_internal( if members_only or not response: return [{self.MEMBER_KEY: member} for member in response] - current_member, current_score = response[0] - current_rank = self.rank_for_in(leaderboard_name, current_member) - current_score = response[0][1] + starting_member, starting_score = response[0] + current_rank = self.rank_for_in(leaderboard_name, starting_member) + current_score = starting_score members = [] ranks_for_members = [] - for index, (member, score) in enumerate(response): - if score != current_score: + for index, (member, member_score) in enumerate(response): + if member_score != current_score: current_rank += 1 - current_score = score + current_score = member_score members.append(member) member_entry = { self.MEMBER_KEY: member, self.RANK_KEY: current_rank, - self.SCORE_KEY: score, + self.SCORE_KEY: member_score, } ranks_for_members.append(member_entry)