CB-244: Schema changes for the rating system #109
Conversation
breakbuidl
commented
Jun 16, 2017
breakbuidl
commented
- Add rating column to revision table to store rating
- Create new table avg_rating to store average rating of an entity
- Add rating column to revision table to store rating - Create new table avg_rating to store average rating of an entity
|
Can one of the admins verify this patch? |
|
@brainzbot, add to whitelist. |
admin/sql/create_primary_keys.sql
Outdated
| @@ -6,6 +6,7 @@ ALTER TABLE oauth_grant ADD CONSTRAINT oauth_grant_pkey PRIMARY KEY (id); | |||
| ALTER TABLE oauth_token ADD CONSTRAINT oauth_token_pkey PRIMARY KEY (id); | |||
| ALTER TABLE review ADD CONSTRAINT review_pkey PRIMARY KEY (id); | |||
| ALTER TABLE revision ADD CONSTRAINT revision_pkey PRIMARY KEY (id); | |||
| ALTER TABLE avg_rating ADD CONSTRAINT avg_rating_pkey PRIMARY KEY (entity_id); | |||
There was a problem hiding this comment.
The PK for avg_rating should be entity_id and entity_type columns.
| @@ -67,7 +67,17 @@ CREATE TABLE revision ( | |||
| id SERIAL NOT NULL, | |||
| review_id UUID, | |||
| "timestamp" TIMESTAMP NOT NULL, | |||
| text VARCHAR NOT NULL | |||
| text VARCHAR, | |||
| rating SMALLINT CHECK (rating >= 0 AND rating <= 100) | |||
There was a problem hiding this comment.
I don't think it's a good idea to make the scale "incorrect" from the start. I think it's better to match it with the way it's going to be used.
Migration of ratings from MusicBrainz is not going to happen any time soon. When it does, we'll be able to easily convert ratings to another scale for storage, if that's even needed. But I'd rather keep it the way it's supposed to be until we need to make these changes.
There was a problem hiding this comment.
Also, is it rating >= 0 or rating >= 1?
There was a problem hiding this comment.
So, it will be rating >= 1 and rating <=5 ( on a scale of 5 as decided in the IRC discussions)
There was a problem hiding this comment.
Migration of ratings from MusicBrainz is not going to happen any time soon. When it does, we'll be able to easily convert ratings to another scale for storage, if that's even needed.
I don't think that's a good reason to actively make it harder to. Converting to MB's scale in the future will require additional engineering time (code changes, a schema change and upgrade script, downtime), when you can just support it now for free. And it will be needed, because you can't express those ratings as an integer from 1-5 (if this were NUMERIC(3, 2) it'd be fine, but you'd be storing a bunch of extra bytes per row for no reason).
There was a problem hiding this comment.
Do we even have ratings in MB that are not either 20, 40, 60, 80, or 100? My main concern is that scales are different and I don't know how we'll support other values set through the WS after they are imported into CB. Having https://tickets.metabrainz.org/browse/CB-248 implemented might help with this.
I'm fine with having the same storage as MBS does, but it doesn't seem right to me.
There was a problem hiding this comment.
They exist, but not in huge numbers (436 release group ratings, 708 recording ratings not on a scale of 1-5).
MB's CSS will display fractional stars just fine (it's needed for the average ratings), though if you click on the stars it can only do 1-5. That said, I don't think that makes 0-100 "incorrect"; we could just as easily decide the number of stars we display is incorrect, or trivially modify the UI to support setting half-stars, etc.
Note that it's possible in MB to give something 0 stars, so there are 101 possible ratings.
There was a problem hiding this comment.
OK, let's use the same way of storing them then.
| CREATE TABLE avg_rating ( | ||
| entity_id UUID NOT NULL, | ||
| entity_type entity_types NOT NULL, | ||
| rating SMALLINT NOT NULL CHECK (rating >= 0 AND rating <= 100), |
There was a problem hiding this comment.
How it this value going to be updated?
There was a problem hiding this comment.
It will be done as a part of db script. Every time, there's a revision with new rating, it will be updated.
There was a problem hiding this comment.
It might be easier to use that with a database trigger instead. I believe @mwiencek suggested that before. Let us know if you need help with that.
|
You might also want to reflect the title of the pull request to indicate which part of rating system is being added since it's not a complete implementation. |
breakbuidl
changed the title
breakbuidl
changed the title
breakbuidl
force-pushed
the
rating-system
branch
from
f86397f
to
09cbe33
Compare
critiquebrainz/db/avg_rating.py
Outdated
| "entity_id": entity_id, | ||
| }) | ||
|
|
||
| def get_avg_rating(entity_id): |
There was a problem hiding this comment.
It would be better to name this function to simply get to be consistent with the rest in this module. It shouldn't be confusing what's being retrieved since it's in the name of the module.
There was a problem hiding this comment.
Also, you probably need an entity_type argument since it's another PK.
|
Can you put changes to the database access code (apart from |
|
I will make a fresh PR with upsert statement and the test script for avg_rating. |
breakbuidl
changed the title
gentlecat
changed the title
