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

Update score table structure #10869

Merged
merged 16 commits into from
Jan 24, 2024
Merged

Update score table structure #10869

merged 16 commits into from
Jan 24, 2024

Conversation

nanaya
Copy link
Collaborator

@nanaya nanaya commented Jan 16, 2024

This uses raw sql for migration due to laravel not supporting float until laravel 11. Also the syntax just not being very helpful in the first place (if anything the migration result has been occasionally changed between laravel versions).

Indexer image points to my own published image because the changes need ppy/osu-elastic-indexer#154 to be properly tested.

I considered making everything flat - as in handling maximum_statistics, statistics, and mods directly from Score model - but it'd involve even more changes dealing with partial ScoreData assignment so maybe later if at all.

  • depends on Convert ScoreRank to enum #10867 (although not sure if that much meaningful being a rather small change by itself)
  • migration entry 2024_01_12_115738_update_scores_table_final

- a bunch of attributes are moved out of the json
- pp and legacy id moved to main scores table
@nanaya nanaya force-pushed the score-structure-v44 branch from a1e9daf to efcdec8 Compare January 16, 2024 06:10
@bdach
Copy link
Contributor

bdach commented Jan 17, 2024

I did some local full-stack testing and this breaks down a bit at the connection to osu-queue-score-statistics via redis.

The shape of the json that osu-queue-score-statistics expects has changed quite a bit in ppy/osu-queue-score-statistics#198 (see https://github.com/ppy/osu-queue-score-statistics/blob/master/osu.Server.Queues.ScoreStatisticsProcessor/Models/SoloScore.cs).

As far as I can tell, the following diff seems to fix:

diff --git a/app/Models/Solo/Score.php b/app/Models/Solo/Score.php
index dcbf823195..dcb382d00e 100644
--- a/app/Models/Solo/Score.php
+++ b/app/Models/Solo/Score.php
@@ -120,10 +120,19 @@ class Score extends Model implements Traits\ReportableInterface
     {
         LaravelRedis::lpush(static::PROCESSING_QUEUE, json_encode([
             'Score' => [
-                'beatmap_id' => $scoreJson['beatmap_id'],
                 'id' => $scoreJson['id'],
-                'ruleset_id' => $scoreJson['ruleset_id'],
                 'user_id' => $scoreJson['user_id'],
+                'beatmap_id' => $scoreJson['beatmap_id'],
+                'ruleset_id' => $scoreJson['ruleset_id'],
+                'has_replay' => $scoreJson['has_replay'],
+                'rank' => $scoreJson['rank'],
+                'passed' => $scoreJson['passed'],
+                'accuracy' => $scoreJson['accuracy'],
+                'max_combo' => $scoreJson['max_combo'],
+                'total_score' => $scoreJson['total_score'],
+                'started_at' => $scoreJson['started_at'],
+                'ended_at' => $scoreJson['ended_at'],
+                'build_id' => $scoreJson['build_id'],
                 // TODO: processor is currently order dependent and requires
                 // this to be located at the end
                 'data' => json_encode($scoreJson),
diff --git a/app/Transformers/ScoreTransformer.php b/app/Transformers/ScoreTransformer.php
index 31188c8ee5..64eeddd0e7 100644
--- a/app/Transformers/ScoreTransformer.php
+++ b/app/Transformers/ScoreTransformer.php
@@ -128,8 +128,10 @@ class ScoreTransformer extends TransformerAbstract
             $ret['build_id'] = $score->build_id;
             $ret['ended_at'] = $score->ended_at_json;
             $ret['has_replay'] = $score->has_replay;
+            $ret['max_combo'] = $score->max_combo;
             $ret['maximum_statistics'] = $data->maximumStatistics;
             $ret['mods'] = $data->mods;
+            $ret['passed'] = $score->passed;
             $ret['pp'] = $score->pp;
             $ret['ruleset_id'] = $score->ruleset_id;
             $ret['started_at'] = $score->started_at_json;

although with a few caveats:

  • processor-side SoloScore has a preserve flag, but exposing that via the transformer seems incorrect (and nothing in osu-queue-score-statistics seems to be reading it anyway?)
  • processor-side SoloScore has a ranked flag, but that is future-proofing and irrelevant for now (it has a sane default and if anything it'll probably be setting the value itself?)
  • processor-side legacy_score_id and legacy_total_score are obviously irrelevant when pushing a lazer score for processing, so omitting them on web side seems correct
  • data serialisation on web side is a bit inefficient since what the web solo score transformer currently does is basically nesting the full object into itself as json, and osu-queue-score-statistics only needs three fields from there (mods, statistics, and maximum_statistics), so you can probably do some php-fu to exclude everything else from that.

The usual disclaimer of "I have barely any idea what I'm doing here" applies.

It expects database row structure so just give it database row structure.
Add new score attributes to legacy model.
Copy link
Collaborator

@notbakaneko notbakaneko left a comment

Choose a reason for hiding this comment

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

score-helper and solo-score-json also need an update since legacy_total_score isn't nullable now and only legacy_score_id is

notbakaneko
notbakaneko previously approved these changes Jan 22, 2024
Copy link
Collaborator

@notbakaneko notbakaneko left a comment

Choose a reason for hiding this comment

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

seems fine 👀

@nanaya
Copy link
Collaborator Author

nanaya commented Jan 23, 2024

This and #10884 can just be merged and deployed (after adding the migration entry)

(again, as the previous one was overwritten by another commit)
@nanaya nanaya merged commit 1202a3e into ppy:master Jan 24, 2024
1 of 3 checks passed
@nanaya nanaya mentioned this pull request Jan 24, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants