Solution#125
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because a critical requirement is missing that blocks repeatable execution. The script must start by dropping the existing database, but at the top of task.sql (around line 1) there is no DROP DATABASE ShopDB; (preferably DROP DATABASE IF EXISTS ShopDB;) before CREATE DATABASE ShopDB;. This violates the task requirement to recreate the schema from scratch and can cause tests to use stale schema/data.
Critical fixes required:
- At the very beginning of
task.sql(line 1), addDROP DATABASE IF EXISTS ShopDB;beforeCREATE DATABASE ShopDB;to ensure a clean rebuild.
Recommended integrity improvements (non-blocking but strongly advised):
- In
Warehousestable, declareCountryID NOT NULLto enforce that every warehouse belongs to a country. - In
ProductInventory, declareProductID NOT NULL,WarehouseID NOT NULL, andWarehouseAmount NOT NULL(considerCHECK (WarehouseAmount >= 0)or an unsigned type) to avoid incomplete records. - Prevent duplicate product/warehouse rows by either using a composite primary key
(ProductID, WarehouseID)or addingUNIQUE (ProductID, WarehouseID).
You’ve done a solid job normalizing the schema and setting up foreign keys; addressing the drop statement will bring it in line with the task’s core requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| ID INT, | ||
| Name VARCHAR(50), | ||
| PRIMARY KEY (ID) | ||
| ID INT AUTO_INCREMENT PRIMARY KEY, |
There was a problem hiding this comment.
The task explicitly requires dropping any existing ShopDB before re-creating it. Add the statement DROP DATABASE ShopDB; at the very start of the script (before CREATE DATABASE ShopDB;). Without this the script does not meet the requirement to recreate the schema from scratch.
| ID INT AUTO_INCREMENT PRIMARY KEY, | ||
| Name VARCHAR(50) NOT NULL, | ||
| Address VARCHAR(100), | ||
| CountryID INT, |
There was a problem hiding this comment.
If every warehouse must belong to a country (as implied by the model), declare CountryID NOT NULL to enforce that relationship at the schema level and avoid warehouses without a country reference.
| ProductID INT, | ||
| WarehouseID INT, | ||
| WarehouseAmount INT, |
There was a problem hiding this comment.
Declare ProductID, WarehouseID and WarehouseAmount as NOT NULL so inventory rows cannot reference missing product/warehouse or have missing quantity values. This enforces referential integrity and prevents incomplete inventory records.
| CREATE TABLE ProductInventory ( | ||
| ID INT, | ||
| ProductName VARCHAR(50), | ||
| ID INT AUTO_INCREMENT PRIMARY KEY, |
There was a problem hiding this comment.
Prevent duplicate inventory rows for the same product+warehouse pair by enforcing a uniqueness constraint at the business-key level. Either replace the surrogate ID with a composite primary key (ProductID, WarehouseID) or add UNIQUE(ProductID, WarehouseID) to ProductInventory so each product/warehouse pair appears only once.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I’m requesting changes because a few integrity constraints required by the checklist are still missing and they impact data correctness.
Critical issues to fix:
- In
task.sql(Warehouses table):CountryID INTis nullable. Per the checklist, every warehouse must belong to a country. Make itCountryID INT NOT NULLand keep the existing FK (e.g., around the Warehouses definition line where columns are declared). - In
task.sql(ProductInventory table):ProductID,WarehouseID, andWarehouseAmountare nullable. Declare themNOT NULL(around the column definitions). Also enforce non-negative stock: useINT UNSIGNEDforWarehouseAmountor addCHECK (WarehouseAmount >= 0)(MySQL 8.0+ supports CHECKs). - In
task.sql(ProductInventory table): Duplicates of(ProductID, WarehouseID)are currently possible because the PK isID. Enforce one row per product-per-warehouse by either making a composite primary keyPRIMARY KEY (ProductID, WarehouseID)or addingUNIQUE (ProductID, WarehouseID)while keeping the surrogateID.
Minor suggestions (optional): consider UNIQUE on Countries.Name if names should be unique, and explicitly choose FK ON DELETE behaviors.
You’ve done solid work normalizing the schema and ordering inserts correctly—addressing the above will lock in data integrity and make tests more reliable. Keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| ID INT AUTO_INCREMENT PRIMARY KEY, | ||
| Name VARCHAR(50) NOT NULL, | ||
| Address VARCHAR(100), | ||
| CountryID INT, |
There was a problem hiding this comment.
CountryID INT is nullable. The checklist recommends that every warehouse belongs to a country — declare this column NOT NULL (e.g., CountryID INT NOT NULL) to enforce referential integrity at the schema level.
| CREATE TABLE ProductInventory ( | ||
| ID INT, | ||
| ProductName VARCHAR(50), | ||
| ID INT AUTO_INCREMENT PRIMARY KEY, |
There was a problem hiding this comment.
ID INT AUTO_INCREMENT PRIMARY KEY in ProductInventory allows duplicate (ProductID, WarehouseID) rows. Enforce one inventory row per product-per-warehouse by either: (a) replacing the auto-increment ID with a composite primary key PRIMARY KEY (ProductID, WarehouseID), or (b) keep the ID but add UNIQUE (ProductID, WarehouseID). This prevents duplicate inventory rows.
| ProductID INT, | ||
| WarehouseID INT, | ||
| WarehouseAmount INT, |
There was a problem hiding this comment.
ProductID, WarehouseID, and WarehouseAmount are nullable. Make them NOT NULL. Also ensure WarehouseAmount cannot be negative — use INT UNSIGNED or add CHECK (WarehouseAmount >= 0) to enforce non-negative quantities.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved. You’ve met the core requirements: the schema is properly normalized (Countries, Warehouses with CountryID FK, Products, and a ProductInventory association with NOT NULLs, INT UNSIGNED, and a unique (ProductID, WarehouseID)), and the INSERT order correctly reproduces the test data.
- At task.sql line with
DROP DATABASE IF EXISTS ShopDB;: if the grader expects the exactDROP DATABASE ShopDB;, consider removingIF EXISTSto match the spec; otherwise your choice is safer for reruns. - At the
Countriestable definition: consider addingUNIQUE (Name)to prevent duplicate country names if that aligns with your domain rules. - At each
FOREIGN KEY(e.g.,Warehouses.CountryIDand both FKs inProductInventory): consider specifying explicitON DELETE/ON UPDATEactions (e.g.,RESTRICT,CASCADE) to clarify intended behavior.
This is an educational approval to help you progress while learning; the above points are optional improvements. Great job on clean structure, constraints, and data integrity; keep it up!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @@ -1,33 +1,51 @@ | |||
| -- Create database and tables | |||
|
|
|||
| DROP DATABASE IF EXISTS ShopDB; | |||
There was a problem hiding this comment.
The script uses DROP DATABASE IF EXISTS ShopDB;. The task description explicitly mentions DROP DATABASE ShopDB;. IF EXISTS is safer for re-runs, but if the automated grader expects the exact statement without IF EXISTS, change this to match the grader. Otherwise IF EXISTS is fine.
| Name VARCHAR(50), | ||
| PRIMARY KEY (ID) | ||
| ID INT AUTO_INCREMENT PRIMARY KEY, | ||
| Name VARCHAR(50) NOT NULL |
There was a problem hiding this comment.
Consider adding a UNIQUE (Name) constraint on Countries.Name if duplicate country names should be prevented in your domain. This is optional (low priority) but can help data quality.
| Name VARCHAR(50) NOT NULL, | ||
| Address VARCHAR(100), | ||
| CountryID INT NOT NULL, | ||
| FOREIGN KEY (CountryID) REFERENCES Countries(ID) |
There was a problem hiding this comment.
Foreign key is defined here; consider specifying explicit ON DELETE/ON UPDATE behavior (e.g., RESTRICT, CASCADE, or NO ACTION) to make referential actions explicit. This is optional but clarifies intended behavior.
| ProductID INT NOT NULL, | ||
| WarehouseID INT NOT NULL, | ||
| WarehouseAmount INT UNSIGNED NOT NULL DEFAULT 0, | ||
| FOREIGN KEY (ProductID) REFERENCES Products(ID), |
There was a problem hiding this comment.
The foreign keys on ProductInventory are correct. As with the Warehouses FK, you may want to specify ON DELETE/ON UPDATE actions for these FKs to document expected cascade or restriction behavior. This is optional and not required by the checklist.
2 participants
No description provided.