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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from Jul 10, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03de33d
Implement schema changes to store ratings
Jun 16, 2017
a71b73f
Change in primary key in avg_rating
Jun 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions admin/schema_changes/8.sql
View file
Open in desktop
@@ -0,0 +1,18 @@
BEGIN;

ALTER TABLE revision ALTER COLUMN text DROP NOT NULL;
ALTER TABLE revision ADD COLUMN rating SMALLINT;

ALTER TABLE revision ADD CONSTRAINT revision_rating_check CHECK (rating >= 0 AND rating <=100);
ALTER TABLE revision ADD CONSTRAINT revision_text_rating_both_not_null_together
CHECK (rating is NOT NULL OR text is NOT NULL);

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),
count INTEGER NOT NULL,
PRIMARY KEY (entity_id, entity_type)
);

COMMIT;
1 change: 1 addition & 0 deletions admin/sql/create_primary_keys.sql
View file
Open in desktop
Expand Up @@ -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, entity_type);
ALTER TABLE spam_report ADD CONSTRAINT spam_report_pkey PRIMARY KEY (user_id, revision_id);
ALTER TABlE "user" ADD CONSTRAINT user_pkey PRIMARY KEY (id);
ALTER TABlE vote ADD CONSTRAINT vote_pkey PRIMARY KEY (user_id, revision_id);
Expand Down
12 changes: 11 additions & 1 deletion admin/sql/create_tables.sql
View file
Open in desktop
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, let's use the same way of storing them then.

);
ALTER TABLE revision ADD CONSTRAINT revision_text_rating_both_not_null_together
CHECK (rating is NOT NULL OR text is NOT NULL);

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),
Copy link
Contributor

Choose a reason for hiding this comment

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

How it this value going to be updated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

count INTEGER NOT NULL
);

CREATE TABLE spam_report (
Expand Down
1 change: 1 addition & 0 deletions admin/sql/drop_tables.sql
View file
Open in desktop
Expand Up @@ -9,4 +9,5 @@ DROP TABLE IF EXISTS moderation_log;
DROP TABLE IF EXISTS review;
DROP TABLE IF EXISTS "user";
DROP TABLE IF EXISTS license;
DROP TABLE IF EXISTS avg_rating;
COMMIT;