Skip to content

fix(sqlserver): preserve IDENTITY columns when copying table (#1691) - #1918

Merged
openai0229 merged 8 commits into
OtterMind:mainfrom
HandSonic:fix/1691-copy-table-identity
Jul 25, 2026
Merged

openai0229 merged 8 commits into
OtterMind:mainfrom
HandSonic:fix/1691-copy-table-identity

Conversation

@HandSonic

@HandSonic HandSonic commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Preserve SQL Server IDENTITY semantics when copying a table while keeping the generated DDL and data-copy SQL valid for real-world schemas.

Changes

  • Create the target table from tableDDL() and rewrite the target schema/table references in table, index, extended-property, and self-referencing foreign-key DDL.
  • Remove explicit constraint names that would collide with the source table.
  • Split SQL Server GO batches without treating strings, comments, or quoted identifiers as separators, including the legacy go<TAB>exec sp_addextendedproperty form.
  • Copy data with the same explicit source/target column list, excluding computed, TIMESTAMP, and ROWVERSION columns.
  • Enable IDENTITY_INSERT only for tables that actually contain an identity column, and always attempt to turn it off after a successful ON, preserving cleanup failures as suppressed exceptions.
  • Preserve non-default identity seed/increment values with BigDecimal, normalize a single bracket-quoted table identifier at the copy boundary, and avoid invalid JDBC size metadata for TIMESTAMP/ROWVERSION.
  • Return referenced schema and table names separately when rebuilding foreign keys; quote and escape constraint/column identifiers; preserve independent ON DELETE and ON UPDATE actions in SQL Server grammar order.
  • Add focused regression coverage for DDL rewriting, batch splitting, column selection, identity handling, identifier quoting, referential actions, and failure cleanup.

Validation

  • Tools: 26 tests passed.
  • SPI: 54 tests passed.
  • SQL Server plugin: 37 tests passed.
  • git diff --check passed.

Related to #1691

@HandSonic HandSonic closed this Jul 21, 2026
@HandSonic HandSonic reopened this Jul 21, 2026

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The current implementation is not safe for the SQL Server table shapes claimed by this PR.

  • SET IDENTITY_INSERT ... ON is executed unconditionally and fails for tables without an identity column.
  • INSERT INTO target SELECT * FROM source is invalid when computed columns or rowversion/timestamp columns are present; the copy needs explicit compatible column lists.
  • Global ddl.replace("[source]", "[target]") can rewrite references outside the object name and does not safely regenerate constraint/index names.

Please redesign this around parsed metadata and explicit column/object lists, then add SQL Server integration tests for tables with no identity, identity, computed columns, rowversion, defaults, constraints, and indexes. The PR should not claim those objects are preserved until the tests prove it.

@HandSonic
HandSonic force-pushed the fix/1691-copy-table-identity branch 5 times, most recently from 8bbb63c to 06fb88f Compare July 24, 2026 09:52
@HandSonic

Copy link
Copy Markdown
Contributor Author

Redesigned the copyTable implementation per review feedback:

1. Conditional IDENTITY_INSERT — no longer executed unconditionally. Now queries sys.columns via SELECT_TABLE_COLUMNS to check if the table has any identity column. Only executes SET IDENTITY_INSERT ON/OFF when identity columns exist.

2. Explicit column list — instead of INSERT INTO target SELECT * FROM source, now builds an explicit column list by querying metadata and filtering out:

  • Computed columns (have COMPUTED_DEFINITION)
  • timestamp/rowversion columns (auto-generated, not copyable)

Uses INSERT INTO target (col1, col2, ...) SELECT col1, col2, ... FROM source.

3. Safe table name replacement — instead of global ddl.replace("[source]", "[target]"), now uses replaceFirst with a regex targeting only the CREATE TABLE [tableName] line, so constraint/index references inside the DDL are not affected.

4. Rebased on current main.

…nd#1691)

Use Chat2DBContext.getDbMetaData().tableDDL() to generate proper CREATE TABLE
DDL that preserves IDENTITY, computed columns, constraints, indexes, etc.

For data copying, wrap INSERT INTO...SELECT with SET IDENTITY_INSERT ON/OFF
to allow explicit values in identity columns.

The previous SELECT * INTO approach silently dropped IDENTITY properties.
@HandSonic
HandSonic force-pushed the fix/1691-copy-table-identity branch from 06fb88f to 3e9f21a Compare July 24, 2026 14:02

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maintainer follow-up now covers explicit column selection, identity ownership and cleanup, SQL-aware DDL/batch rewriting, identifier handling, and focused regression tests. The previous blocking concerns are resolved.

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maintainer follow-up complete: verified identifier normalization, constraint quoting, referential-action preservation, and SQL Server grammar ordering. Tools (26), SPI (54), and SQL Server (37) tests pass.

@openai0229
openai0229 merged commit e778f9c into OtterMind:main Jul 25, 2026
19 checks passed
@openai0229 openai0229 moved this from In Review to Done in Chat2DB Community Jul 25, 2026
@HandSonic
HandSonic deleted the fix/1691-copy-table-identity branch August 23, 2026 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants