Revert "Make enrichment_pipeline_runs.status NOT NULL with default 'running'"#152
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Free Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note 🎁 Summarized by CodeRabbit FreeThe PR author is not assigned a seat. To perform a comprehensive line-by-line review, please assign a seat to the pull request author through the subscription management page by visiting https://app.coderabbit.ai/login. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request reverts a previous change that added a Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
d05a377
into
install-skill-67990
There was a problem hiding this comment.
Pull request overview
This PR reverts PR #151, which had made the enrichment_pipeline_runs.status column NOT NULL with a default of 'running'. The revert removes both the NOT NULL constraint and the DEFAULT 'running' clause, keeping only the CHECK constraint.
Changes:
- Reverts the
statuscolumn inenrichment_pipeline_runsto be nullable with no default value
There was a problem hiding this comment.
Code Review
This pull request reverts a change that made the enrichment_pipeline_runs.status column NOT NULL with a default value. While this might be a necessary short-term fix, my review points out that making the status column nullable is a potential data integrity risk. I've suggested alternative approaches that would solve the underlying issue without allowing NULL values, such as keeping the NOT NULL constraint but removing the default, or adding a new explicit initial state.
| products_failed INTEGER DEFAULT 0, | ||
| errors JSONB DEFAULT '[]', | ||
| status TEXT NOT NULL DEFAULT 'running' CHECK (status IN ('running', 'completed', 'failed', 'partial')), | ||
| status TEXT CHECK (status IN ('running', 'completed', 'failed', 'partial')), |
There was a problem hiding this comment.
While this revert may fix an immediate problem, making the status column nullable introduces potential data integrity issues and ambiguity. A NULL status is hard to interpret and complicates queries, as they need to handle NULL explicitly.
It's a database design best practice to keep status columns NOT NULL. To resolve the issue that prompted this revert without sacrificing data integrity, please consider one of the following alternatives:
-
Keep
NOT NULLand removeDEFAULT: This would enforce that everyINSERToperation explicitly provides a status, which could help uncover issues in client code.status TEXT NOT NULL CHECK (status IN ('running', 'completed', 'failed', 'partial')),
-
Introduce an explicit initial state: If a state before 'running' is needed, it's better to add it to the
CHECKconstraint rather than usingNULL.status TEXT NOT NULL DEFAULT 'pending' CHECK (status IN ('pending', 'running', 'completed', 'failed', 'partial')),
These approaches maintain a clear and unambiguous state for every pipeline run.
Reverts #151