Skip to content

Conversation

@jasonyess
Copy link
Contributor

Modified the ranked_players view to include scoreless players and give them a null rank
To include scoreless players in the response of /players/ranking, a scoreless query parameter must be provided and set to true (to not break anything which may rely on ranks not being null)

Resolves #132

License Acceptance

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov
Copy link

codecov bot commented Mar 23, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 36.15%. Comparing base (fb99bc7) to head (0832b30).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #234      +/-   ##
==========================================
- Coverage   36.16%   36.15%   -0.01%     
==========================================
  Files         121      121              
  Lines        5837     5838       +1     
==========================================
  Hits         2111     2111              
- Misses       3726     3727       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@stadust
Copy link
Owner

stadust commented Mar 23, 2025

Mhh, this isn't what we were going for, sorry 😅 What #132 wanted to do was to add only those players back to the stats viewer that have verified a legacy demon, or have accepted records on the legacy list - just adding all players will be a huge mess (just go through the pages returned by GET /players, there a lot of spam in there). And those players are fine if they get some non-null rank at the end of the stats viewer

@jasonyess
Copy link
Contributor Author

aaaaaaaa oops
to clarify the only players that should be listed are those with at least one approved legacy record/verification

@stadust
Copy link
Owner

stadust commented Mar 23, 2025

aaaaaaaa oops to clarify the only players that should be listed are those with at least one approved legacy record/verification

yes!! Sorry, the issue wasn't very descriptive 😭

@jasonyess
Copy link
Contributor Author

added extra conditions, this should do the trick 😞
personally i think having the ranks be null is better, but i dont mind changing it if youre against it

Copy link
Owner

@stadust stadust left a comment

Choose a reason for hiding this comment

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

We can't query the records table for each individual row of the players table - the production tables are just way too large for this to be efficient. E.g. with your new ranked_players view a simple select * from ranked_players limit 50 goes from 4ms to 400ms - that's 10000% slower (and I'm only running this on a local copy of the database from early last year, so in practice it'd probably be even worse). It'd make the server crawl to a halt I'm afraid.

The determination of including players with 0 points needs to happen when the score is initially computed, e.g. inside the recompute_*_score[s] stored procedures which populate the score column.

I do like the idea of using NULL vs 0.0 for separating out players that should and shouldn't be displayed though. E.g. setting score = 0.0 for players that only have legacy records/verifications or are creators, and setting everyone else to NULL instead. Then these queries can just filter out the players with NULL score.

I'll insist on rank being non-nullable though. That's how the endpoint is documented

@jasonyess
Copy link
Contributor Author

i can't believe performance hasnt crossed my mind once
gonna need to rethink this one lol

@jasonyess jasonyess requested a review from stadust March 27, 2025 21:46
@jasonyess
Copy link
Contributor Author

jasonyess commented Mar 27, 2025

new changes are purely logical so there shouldnt be any major performance issues haha
scores are now nullable; if the player does not appear in the score_giving table, the player will have a null score
this method relies on record_score returning a non-null score (0) if the provided record is not worth any points

Make "score" nullable, where score == 0 means "this player has approved
records, verifications, but they do not give score anymore because they
are legacy or progress records on extended demons", and score == NULL
means "this player has no approved records or verifications whatsoever".

Then have the stats viewer query all players that have non-NULL score,
so that players with only legacy records (e.g. RioT) show up again.

Signed-off-by: stadust <[email protected]>
@stadust stadust force-pushed the list-scoreless-players branch from 50128e9 to b1c343e Compare August 11, 2025 22:24
@stadust stadust dismissed their stale review August 11, 2025 22:25

impl good now (am looking into test failures)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

List players with zero points but accepted legacy records on the stats viewer

2 participants