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

CB-244: Schema changes for the rating system #109

Merged
merged 2 commits into from Jul 10, 2017

Conversation

@psolanki10
Copy link
Contributor

psolanki10 commented Jun 16, 2017

  • 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
@brainzbot

This comment has been minimized.

Copy link
Member

brainzbot commented Jun 16, 2017

Can one of the admins verify this patch?

@gentlecat

This comment has been minimized.

Copy link
Contributor

gentlecat commented Jun 16, 2017

@brainzbot, add to whitelist.

@@ -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);

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 19, 2017

Contributor

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)

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 19, 2017

Contributor

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.

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 19, 2017

Contributor

Also, is it rating >= 0 or rating >= 1?

This comment has been minimized.

Copy link
@psolanki10

psolanki10 Jun 20, 2017

Author Contributor

So, it will be rating >= 1 and rating <=5 ( on a scale of 5 as decided in the IRC discussions)

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jun 20, 2017

Member

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).

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 20, 2017

Contributor

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.

This comment has been minimized.

Copy link
@mwiencek

mwiencek Jun 20, 2017

Member

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.

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 20, 2017

Contributor

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),

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 19, 2017

Contributor

How it this value going to be updated?

This comment has been minimized.

Copy link
@psolanki10

psolanki10 Jun 20, 2017

Author Contributor

It will be done as a part of db script. Every time, there's a revision with new rating, it will be updated.

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 20, 2017

Contributor

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.

@gentlecat

This comment has been minimized.

Copy link
Contributor

gentlecat commented Jun 19, 2017

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.

@psolanki10 psolanki10 changed the title CB-244: Implement a rating system CB-244: Rating System (Schema changes) Jun 20, 2017
@psolanki10 psolanki10 changed the title CB-244: Rating System (Schema changes) CB-244: Rating System (Schema changes and SQL queries) Jun 26, 2017
@psolanki10 psolanki10 force-pushed the psolanki10:rating-system branch from f86397f to 09cbe33 Jun 27, 2017
Copy link
Contributor

gentlecat left a comment

I think functions create_or_update, create, and update can be reduced to one that updates average rating of an entity (given entity ID and entity type) using an upsert statement.

"entity_id": entity_id,
})

def get_avg_rating(entity_id):

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 28, 2017

Contributor

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.

This comment has been minimized.

Copy link
@gentlecat

gentlecat Jun 28, 2017

Contributor

Also, you probably need an entity_type argument since it's another PK.

This comment has been minimized.

Copy link
@psolanki10

psolanki10 Jul 5, 2017

Author Contributor

Changes noted.

@gentlecat

This comment has been minimized.

Copy link
Contributor

gentlecat commented Jun 28, 2017

Can you put changes to the database access code (apart from critiquebrainz/db/avg_rating.py) into a separate pull request based on this one? Let's take this one step at a time and try not to add too many things too quickly.

@psolanki10

This comment has been minimized.

Copy link
Contributor Author

psolanki10 commented Jul 5, 2017

I will make a fresh PR with upsert statement and the test script for avg_rating.

@psolanki10 psolanki10 force-pushed the psolanki10:rating-system branch from 09cbe33 to 8da5bc9 Jul 9, 2017
@psolanki10 psolanki10 changed the title CB-244: Rating System (Schema changes and SQL queries) CB-244: Rating System (Schema changes) Jul 9, 2017
@psolanki10 psolanki10 force-pushed the psolanki10:rating-system branch from 8da5bc9 to a71b73f Jul 9, 2017
@gentlecat gentlecat changed the title CB-244: Rating System (Schema changes) CB-244: Schema changes for the rating system Jul 10, 2017
@gentlecat gentlecat merged commit 8698fc7 into metabrainz:master Jul 10, 2017
1 check passed
1 check passed
Jenkins Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.