Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Goose SQL migration that renames and recreates ChangesData Quality Stats Table Migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sirisjo
changed the title
data_quality_stats table BED-8616data_quality_stats table BED-8616
sirisjo
changed the title
data_quality_stats table BED-8616data_quality_stats table BED-8616
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@cmd/api/src/database/migration/migrations/20260630135900_v9_drop_and_recreate_data_quality_stats_BED-8616.sql`:
- Around line 1-40: The migration filename does not follow the required
snake_case convention because it uses an uppercase hyphenated ticket suffix.
Rename the migration to match the existing pattern used by the v9
data_quality_stats migrations, keeping the timestamp and version prefix intact
while changing the descriptive name to snake_case (for example, use a lowercase
ticket suffix like in 20260622132700_v9_add_data_quality_stats_bed_8616.sql).
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 69007cf9-e4ed-4d12-944a-3bc9606b66bc
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/20260630135900_v9_drop_and_recreate_data_quality_stats_BED-8616.sql
| -- +goose Up | ||
|
|
||
| -- Drop the old data_quality_stats table that is not currently used anywhere but still exists in prod | ||
| DROP TABLE IF EXISTS data_quality_stats; |
There was a problem hiding this comment.
Should we instead rename/archive the table instead of deleting it?
There was a problem hiding this comment.
renamed to data_quality_stats_old 🫡
sirisjo
changed the title
data_quality_stats table BED-8616data_quality_stats table BED-8616
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@cmd/api/src/database/migration/migrations/20260630135900_v9_drop_and_recreate_data_quality_stats.sql`:
- Around line 19-20: The migration currently renames data_quality_stats but
leaves the existing sequence and primary-key index behind on the old table,
which can conflict with the new table creation. Update the migration around the
ALTER TABLE rename and the subsequent CREATE TABLE for data_quality_stats so the
old sequence and PK index are also renamed/dropped, or define explicit names for
the new table’s serial sequence and primary key index. Use the existing
migration block and the new data_quality_stats table definition as the anchor
points 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e70308de-ef98-43dc-be9d-64c71e5da544
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/20260630135900_v9_drop_and_recreate_data_quality_stats.sql
mvlipka
left a comment
mvlipka
left a comment
There was a problem hiding this comment.
Verified the indexes are correct and that we don't have an associated aggregation table in test
Great catch!
3 participants
Description
Upon adding this table in a previous release, we discovered that there exists an old
data_quality_statstable in the test and production environments from a long time ago that is not tracked in any of our migration files. When running the migration in the test environment, the new table was not created because thedata_quality_statstable already existed. We need to drop that old table first, and then we can recreate it with the new columns that we want.Motivation and Context
Resolves BED-8616
Why is this change required? What problem does it solve?
How Has This Been Tested?
Ran the migration locally.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit