Skip to content

Conversation

@sam-mosleh
Copy link
Contributor

Fixes #277

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the online rewrite logic for column SET NOT NULL operations when the same column diff also produces other statements (e.g., SET DEFAULT), resolving the duplicated work / missing default tracking described in issue #277.

Changes:

  • Update generateRewrite for DiffTypeTableColumn to only apply the NOT NULL rewrite when the current diff’s SQL actually contains SET NOT NULL, avoiding duplicate rewrite application when multiple statements exist for the same column.
  • Add regression fixtures under testdata/diff/create_table/add_default_not_null to cover a table column transitioning from nullable to NOT NULL while also adding a default, with the expected plan including the check-constraint-based online rewrite and a single SET DEFAULT.
  • Ensure the generated plan JSON, SQL, and human-readable plan text accurately reflect the corrected behavior for this scenario.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/plan/rewrite.go Narrows the column NOT NULL rewrite to only diffs whose SQL contains SET NOT NULL, preventing duplicate rewrite steps when a ColumnDiff emits both SET NOT NULL and SET DEFAULT.
testdata/diff/create_table/add_default_not_null/old.sql Defines the original people table with a nullable created_at column for the regression scenario.
testdata/diff/create_table/add_default_not_null/new.sql Defines the target schema where created_at is DEFAULT NOW() NOT NULL, exercising both default and NOT NULL changes.
testdata/diff/create_table/add_default_not_null/diff.sql Captures the canonical diff SQL (SET NOT NULL then SET DEFAULT now()) that should be transformed by the rewrite logic.
testdata/diff/create_table/add_default_not_null/plan.sql Asserts the exact SQL sequence for the online NOT NULL rewrite followed by setting the default, ensuring no duplicated NOT NULL workflow.
testdata/diff/create_table/add_default_not_null/plan.txt Snapshot of the human-readable plan confirming a single column modification with the correct DDL sequence.
testdata/diff/create_table/add_default_not_null/plan.json Machine-readable plan snapshot validating the grouped steps, operations, and paths for the corrected NOT NULL + default behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@tianzhou tianzhou left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution.

@tianzhou tianzhou merged commit b884c60 into pgplex:main Feb 1, 2026
7 checks passed
@sam-mosleh sam-mosleh deleted the fix-adding-default-property branch February 1, 2026 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Can't track defaults properly and double the work is being applied

2 participants