-
Notifications
You must be signed in to change notification settings - Fork 6
Vlad-2week-DB #13
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
base: main
Are you sure you want to change the base?
Vlad-2week-DB #13
Conversation
yunchen4
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just left some suggestions and possible blind spots.
| h_index SMALLINT CHECK (h_index >= 0), | ||
| gender TEXT CHECK (gender IN ('male','female','nonbinary','other')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of SMALLINT for h_index here. For gender, you can also use SMALLINT as it is like enum. The disadvantage of using SMALLINT is that it is not so readable (when checking the table contents, people don't know 1 is female or male). But it is a common practice.
| database: "research", | ||
| }); | ||
|
|
||
| async function q(label, sql, params = []) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: have a better name for the function, so when reading the code we know what this function is for easily.
| await q( | ||
| "Total papers by female authors", | ||
| `SELECT SUM(cnt) AS total_papers_by_female | ||
| FROM ( | ||
| SELECT a.author_id, COUNT(ap.paper_id) AS cnt | ||
| FROM authors a | ||
| LEFT JOIN author_papers ap ON ap.author_id = a.author_id | ||
| WHERE a.gender = 'female' | ||
| GROUP BY a.author_id | ||
| ) x;` | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: If a paper has two different female authors, this paper will be counted twice. Depending on the business requirements, this might be what we want. If you don't want to count the same paper twice, you need to change the query (use multiple JOIN instead of sub query).
| `SELECT a.university, COUNT(ap.paper_id) AS papers_sum | ||
| FROM authors a | ||
| LEFT JOIN author_papers ap ON ap.author_id = a.author_id | ||
| GROUP BY a.university | ||
| ORDER BY papers_sum DESC, a.university;` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the female author query, this will count the same paper twice if it has several authors from the same university.
No description provided.