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

w2-aziz-databases #41

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

azizsigar
Copy link

No description provided.

Copy link

@dyaboykyl dyaboykyl left a comment

Choose a reason for hiding this comment

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

The code looks really good overall, but the relationship between authors and papers needs to be updated

paper_title VARCHAR(255) NOT NULL,
conference VARCHAR(255),
publish_date DATE,
author_id INT,

Choose a reason for hiding this comment

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

This approach only allows a research paper to have one author, but in real life research papers usually have multiple authors. Can you modify the code to support a many-many relationship between papers and authors?

Copy link
Author

Choose a reason for hiding this comment

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

  PRIMARY KEY (paper_id, author_id),

i added primary key. i think it will be solve the problem.

Choose a reason for hiding this comment

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

A primary key that includes author_id and paper_id doesn't still doesn't support the many-to-many relationship without having many duplicate rows. to support a many-to-many relationship you need to create a relationship table that only contains two fields: paper_id and author_id

Here's some reading on that that might help
https://www.geeksforgeeks.org/relationships-in-sql-one-to-one-one-to-many-many-to-many/#:~:text=3.%20Many%2Dto%2DMany%20Relationship

Comment on lines 72 to 78
const authorId = Math.floor(Math.random() * 15) + 1; // Random author_id between 1 and 15
papers.push([
`Research Paper ${i}`,
`Conference ${i}`,
`2023-01-${i}`,
authorId,
]);

Choose a reason for hiding this comment

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

Nice, this is a really elegant way to create test data

@dyaboykyl dyaboykyl self-assigned this Nov 2, 2024
@azizsigar azizsigar requested a review from dyaboykyl November 5, 2024 08:55
console.log("Authors and their Mentors:");
console.table(authorsWithMentors);

// 2. Authors and their published papers

Choose a reason for hiding this comment

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

Great job fixing the many-to-many relationship! Now queries 2,3,4, and 6 need to be updated to support your the new schema. Remember that research_papers table no longer has an author_id field

@azizsigar azizsigar requested a review from dyaboykyl November 19, 2024 11:42
Copy link

@dyaboykyl dyaboykyl left a comment

Choose a reason for hiding this comment

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

Huge improvement!


// 4. Sum of the research papers published by all female authors
const femaleAuthorsPapersCountQuery = `
SELECT COUNT(*) AS FemaleAuthorsPapers

Choose a reason for hiding this comment

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

Careful that you don't double count papers that have multiple female authors. To handle that you can use the DISTINCT keyword as in COUNT(DISTINCT(ap.paper_id))


// 6. Sum of the research papers of the authors per university
const sumPapersPerUniversityQuery = `
SELECT a.university, COUNT(ap.paper_id) AS TotalPapers

Choose a reason for hiding this comment

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

Here as well you need to avoid double counting the same paper if it had multiple authors that are from the same university

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants