data transformation : bronze -> silver data cleaning and standardization in store table #72
Conversation
…ion in store table
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
📝 WalkthroughWalkthroughThis PR refines two SQL data-cleaning scripts: minor formatting adjustments to review channel and title selection in the reviews script, and comprehensive data profiling and standardization additions to the stores script, including new normalization for geographic, text, date, numeric, and boolean fields with consistent ChangesReviews Script Formatting
Stores Script Data Cleaning Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Failed to generate code suggestions for PR |
Ritik574-coder
left a comment
Ritik574-coder
left a comment
There was a problem hiding this comment.
data transformation and cleaning and standardization in store table
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 92a5d03403
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| WHEN opened_date LIKE '__/__/____' AND TRY_CONVERT(INT, SUBSTRING(opened_date, 4, 2)) > 12 THEN TRY_CONVERT(DATE, opened_date, 101) | ||
| WHEN opened_date LIKE '__/__/____' AND TRY_CONVERT(INT, LEFT(opened_date, 2)) > 12 THEN TRY_CONVERT(DATE, opened_date, 103) | ||
| WHEN opened_date LIKE '__-__-____' AND TRY_CONVERT(INT, SUBSTRING(opened_date, 4, 2)) > 12 THEN TRY_CONVERT(DATE, opened_date, 110) | ||
| WHEN opened_date LIKE '__-__-____' AND TRY_CONVERT(INT, LEFT(opened_date, 2)) > 12 THEN TRY_CONVERT(DATE, opened_date, 105) | ||
| ELSE TRY_CONVERT(DATE, opened_date) |
There was a problem hiding this comment.
The opened_date normalizer falls back to TRY_CONVERT(DATE, opened_date) when both day and month are <= 12, so values like 11/07/2015 and 03-12-2019 are interpreted according to the SQL Server session LANGUAGE/DATEFORMAT instead of a fixed rule. This makes the cleaned date output environment-dependent and can silently swap month/day across runs, which corrupts downstream analytics that treat opened_date as standardized.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
explore_database/stores/stores.sql (2)
650-652: ⚡ Quick winInconsistent CR/LF handling across boolean columns.
has_cafecleaning (lines 739-740) stripsCHAR(13)andCHAR(10)before comparison, butis_activeandhas_parkingdo not. If any boolean column data contains embedded newlines, the latter two would incorrectly map to'Unknown'. Consider applying consistent normalization.Proposed fix: Apply consistent CR/LF handling
-- is_active column cleaning and standardization WITH clean_is_active AS ( SELECT CASE - WHEN TRIM(LOWER(is_active)) IN ('true', 'yes', 'y', '1', 'active') THEN 'True' - WHEN TRIM(LOWER(is_active)) IN ('false', 'no', 'n', '0', 'not active') THEN 'False' + WHEN REPLACE(REPLACE(TRIM(LOWER(is_active)), CHAR(13), ''),CHAR(10), '') IN ('true', 'yes', 'y', '1', 'active') THEN 'True' + WHEN REPLACE(REPLACE(TRIM(LOWER(is_active)), CHAR(13), ''),CHAR(10), '') IN ('false', 'no', 'n', '0', 'not active') THEN 'False' ELSE 'Unknown' END as is_active FROM bronze.stores )Apply the same pattern to
has_parking.Also applies to: 694-697
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@explore_database/stores/stores.sql` around lines 650 - 652, The boolean normalization for columns is inconsistent: has_cafe uses TRIM(REPLACE(..., CHAR(13), ''))/CHAR(10) before LOWER but is_active and has_parking do not, so embedded CR/LF can cause wrong 'Unknown' mapping; update the CASE/WHEN blocks that normalize is_active and has_parking (and the other boolean normalization occurrences around the referenced area) to apply the same CR/LF stripping and trimming pattern used by has_cafe before LOWER and comparison so all boolean columns use consistent normalization.
479-483: 💤 Low valueAmbiguous date format falls through to server default.
When both date components are ≤ 12 (e.g.,
05/06/2020), neither disambiguation condition matches andTRY_CONVERT(DATE, opened_date)uses server locale settings. Consider adding a comment documenting the assumed default format or explicitly choosing a style for the ambiguous case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@explore_database/stores/stores.sql` around lines 479 - 483, The CASE that normalizes opened_date can fall back to server-default parsing for ambiguous values (e.g., "05/06/2020") because the ELSE uses TRY_CONVERT(DATE, opened_date) without a style; update the ELSE branch to explicitly convert using a chosen style (e.g., CONVERT/TRY_CONVERT with style 101/103/110/105) or implement a deterministic disambiguation rule, and add a short comment above the CASE noting which format is being assumed for ambiguous MM/DD vs DD/MM inputs so future readers know the chosen behavior; reference the opened_date field and the TRY_CONVERT calls in this CASE when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@explore_database/stores/stores.sql`:
- Line 426: The profiling check uses LEN(TRIM(manager_name)) < 3 while the
cleaning rule uses LEN(TRIM(manager_name)) < 2, causing 2-character names to be
profiled as bad but not cleaned; update one of these to match the other so both
profiling and cleaning use the same minimum length threshold for manager_name
(e.g., change the profiling condition or the cleaning condition so both use
LEN(TRIM(manager_name)) < 3 or both use < 2), and ensure any related conditions
referencing manager_name in this file are adjusted to the same threshold for
consistency.
- Line 875: The SQL has an invalid table reference ".[bronze].[stores]" (also
duplicated at the earlier occurrence) — remove the leading dot and use a valid
qualifier such as "[bronze].[stores]" or supply the full three-part name
"[DatabaseName].[bronze].[stores]" (or "[DatabaseName].[Schema].[stores]" /
"dbo.[stores]" as appropriate); update both occurrences to a properly-qualified
table reference so the FROM clause is syntactically correct.
- Around line 395-401: Remove the duplicated "district column cleaning" block:
delete the repeated comment header lines starting with
"--===================================== district column cleaning
==============================" and the redundant query "SELECT district FROM
bronze.stores;" (the duplicate section shown in the diff). Keep only the
original district cleaning section (the earlier block between the first matching
headers) to avoid repeated headers/queries.
---
Nitpick comments:
In `@explore_database/stores/stores.sql`:
- Around line 650-652: The boolean normalization for columns is inconsistent:
has_cafe uses TRIM(REPLACE(..., CHAR(13), ''))/CHAR(10) before LOWER but
is_active and has_parking do not, so embedded CR/LF can cause wrong 'Unknown'
mapping; update the CASE/WHEN blocks that normalize is_active and has_parking
(and the other boolean normalization occurrences around the referenced area) to
apply the same CR/LF stripping and trimming pattern used by has_cafe before
LOWER and comparison so all boolean columns use consistent normalization.
- Around line 479-483: The CASE that normalizes opened_date can fall back to
server-default parsing for ambiguous values (e.g., "05/06/2020") because the
ELSE uses TRY_CONVERT(DATE, opened_date) without a style; update the ELSE branch
to explicitly convert using a chosen style (e.g., CONVERT/TRY_CONVERT with style
101/103/110/105) or implement a deterministic disambiguation rule, and add a
short comment above the CASE noting which format is being assumed for ambiguous
MM/DD vs DD/MM inputs so future readers know the chosen behavior; reference the
opened_date field and the TRY_CONVERT calls in this CASE when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 05f007ea-01b7-4e71-a899-d431ccb81207
📒 Files selected for processing (2)
explore_database/reviews/reviews.sqlexplore_database/stores/stores.sql
| --============================================================================================= | ||
| --===================================== district column cleaning ============================== | ||
| --============================================================================================= | ||
| -- district column data overview | ||
| SELECT | ||
| district | ||
| FROM bronze.stores; |
There was a problem hiding this comment.
Duplicate section header and query for district.
Lines 351-393 already contain the complete district column cleaning section. This appears to be a copy-paste artifact that should be removed.
Proposed fix: Remove duplicate section
---=============================================================================================
---===================================== district column cleaning ==============================
---=============================================================================================
--- district column data overview
-SELECT
- district
-FROM bronze.stores;
-📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --============================================================================================= | |
| --===================================== district column cleaning ============================== | |
| --============================================================================================= | |
| -- district column data overview | |
| SELECT | |
| district | |
| FROM bronze.stores; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@explore_database/stores/stores.sql` around lines 395 - 401, Remove the
duplicated "district column cleaning" block: delete the repeated comment header
lines starting with "--===================================== district column
cleaning ==============================" and the redundant query "SELECT
district FROM bronze.stores;" (the duplicate section shown in the diff). Keep
only the original district cleaning section (the earlier block between the first
matching headers) to avoid repeated headers/queries.
| WHERE manager_name IS NULL | ||
| OR manager_name != TRIM(manager_name) | ||
| OR TRIM(manager_name) = '' | ||
| OR LEN(TRIM(manager_name)) < 3 ; |
There was a problem hiding this comment.
Inconsistent minimum length threshold for manager_name.
The profiling query (line 426) uses LEN(TRIM(manager_name)) < 3 while the cleaning logic (line 433) uses LEN(TRIM(manager_name)) < 2. This inconsistency means 2-character names would be flagged during profiling but not cleaned to 'Unknown'.
Proposed fix: Align thresholds
-- manager_name column data profiling
SELECT
manager_name
FROM bronze.stores
WHERE manager_name IS NULL
OR manager_name != TRIM(manager_name)
OR TRIM(manager_name) = ''
- OR LEN(TRIM(manager_name)) < 3 ;
+ OR LEN(TRIM(manager_name)) < 2 ;Also applies to: 433-433
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@explore_database/stores/stores.sql` at line 426, The profiling check uses
LEN(TRIM(manager_name)) < 3 while the cleaning rule uses LEN(TRIM(manager_name))
< 2, causing 2-character names to be profiled as bad but not cleaned; update one
of these to match the other so both profiling and cleaning use the same minimum
length threshold for manager_name (e.g., change the profiling condition or the
cleaning condition so both use LEN(TRIM(manager_name)) < 3 or both use < 2), and
ensure any related conditions referencing manager_name in this file are adjusted
to the same threshold for consistency.
| WHEN REPLACE(REPLACE(TRIM(LOWER(has_cafe)), CHAR(13), ''),CHAR(10), '') IN ('0', 'no', 'n','false') THEN 'False' | ||
| ELSE 'Unknown' | ||
| END as has_cafe | ||
| FROM .[bronze].[stores] |
There was a problem hiding this comment.
Syntax error: Invalid leading dot in table reference.
.[bronze].[stores] is invalid SQL Server syntax. The leading dot suggests a database qualifier that is missing. The same issue exists in line 30.
Proposed fix
- FROM .[bronze].[stores]
+ FROM [bronze].[stores]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| FROM .[bronze].[stores] | |
| FROM [bronze].[stores] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@explore_database/stores/stores.sql` at line 875, The SQL has an invalid table
reference ".[bronze].[stores]" (also duplicated at the earlier occurrence) —
remove the leading dot and use a valid qualifier such as "[bronze].[stores]" or
supply the full three-part name "[DatabaseName].[bronze].[stores]" (or
"[DatabaseName].[Schema].[stores]" / "dbo.[stores]" as appropriate); update both
occurrences to a properly-qualified table reference so the FROM clause is
syntactically correct.
1 participant
Summary by CodeRabbit
Refactor
Chores