Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix 4837 alter table alter column #4846

Merged

Conversation

griffio
Copy link
Contributor

@griffio griffio commented Nov 27, 2023

fixes #4837

🚧 🐘 🍔 Working on fix

Allow AlterTableAlterColumnMixin extends SqlColumnDef to provide operations to TableInterfaceGenerator as it now becomes a ColumnDefMixin source - however it only seems viable for Column Type changes

Override SqlColumnDef members ( use the column_type declared in the ALTER TABLE or existing SqlColumnDef declared in CREATE TABLE ) to fall back to the current values if they are not being altered. Only column_type and column_name can be used with SqlColumnDef. ColumnConstraints are not used to drop/set nullability (use query column nullable) as there is no matching clause in ALTER TABLE.

FIXED - If a migration step 1 is ALTER TABLE test ALTER COLUMN first DROP NOT NULL and step 2 is ALTER TABLE test ALTER COLUMN first TYPE BIGINT; changing the type propagates the same NOT NULL/NULL - keep the current queryColumn.nullable value when not a null value - means it was set in another migration and queryColumn propagates the correct nullability
e.g queryColumn.copy(element = alterColumnTable, nullable = nullableColumn ?: queryColumn.nullable)

🦺 Performed more testing of local snapshot with column type changes and nullability changes

1.sqm

CREATE TABLE X(
    y INTEGER NOT NULL,
    t TEXT,
    a INTEGER NOT NULL,
    b TEXT
);

2.sqm

ALTER TABLE X
ALTER COLUMN a TYPE BIGINT;

3.sqm

ALTER TABLE X
ALTER COLUMN y TYPE BIGINT;

4.sqm

ALTER TABLE X
ALTER COLUMN y DROP NOT NULL;

5.sqm

ALTER TABLE X
ALTER COLUMN t SET NOT NULL;

results in

public data class X(
  public val y: Long?,
  public val t: String,
  public val a: Long,
  public val b: String?,
)

@griffio griffio force-pushed the fix-4837-alter-table-alter-column branch 2 times, most recently from ec77ff5 to 6991e82 Compare November 28, 2023 20:54
…th TableInterfaceGenerator.

getColumnConstraintList - use create table constraints - nullability is override in queryColumn

getColumnName - use alter table column name

getColumnType - use alter table column data type if present or use create table column def

getJavadoc - use create table defaults - not used
Add column and expect type to be migrated from Int (Integer) to Long (BIGINT)
Second version to enable passing tests

TODO - improvements
Changing the column type must propagate the existing not null/null constraint
Change fifth column to nullable
Change fifth column to BIGINT (Long)
Expect Long?
getColumnConstraintList return null seems to work as is not used
@griffio griffio force-pushed the fix-4837-alter-table-alter-column branch from 14fcf7c to 19e39bc Compare November 29, 2023 15:43
@griffio griffio marked this pull request as ready for review November 29, 2023 16:26
@griffio griffio marked this pull request as draft November 29, 2023 18:54
@griffio griffio marked this pull request as ready for review November 30, 2023 14:36
Copy link
Collaborator

@AlecKazakova AlecKazakova left a comment

Choose a reason for hiding this comment

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

looks great, thank you!

@AlecKazakova AlecKazakova merged commit 03c7f1e into cashapp:master Nov 30, 2023
7 checks passed
@griffio griffio deleted the fix-4837-alter-table-alter-column branch November 30, 2023 16:51
hfhbd pushed a commit that referenced this pull request Apr 2, 2024
* AlterTableAlterColumnMixin extends SqlColumnDef to allow operating with TableInterfaceGenerator.

getColumnConstraintList - use create table constraints - nullability is override in queryColumn

getColumnName - use alter table column name

getColumnType - use alter table column data type if present or use create table column def

getJavadoc - use create table defaults - not used

* AlterTableAlterColumnMixin tests migration
Add column and expect type to be migrated from Int (Integer) to Long (BIGINT)

* Fix the null elements

Second version to enable passing tests

TODO - improvements

* Propagate the nullable type

Changing the column type must propagate the existing not null/null constraint

* update tests

Change fifth column to nullable
Change fifth column to BIGINT (Long)
Expect Long?

* AlterTableAlterColumnMixin columnConstraintList

getColumnConstraintList return null seems to work as is not used

* getColumnConstraintList returns constraints

reinstate after testing
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.

Support PostgreSql ALTER TABLE table_name ALTER COLUMN column_name TYPE new_data_type
2 participants