Skip to content

feat(core): Improve escaping in destinations and rework state structure#771

Merged
iambriccardo merged 8 commits into
mainfrom
improve-code
May 27, 2026
Merged

feat(core): Improve escaping in destinations and rework state structure#771
iambriccardo merged 8 commits into
mainfrom
improve-code

Conversation

@iambriccardo

@iambriccardo iambriccardo commented May 27, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR improves destination SQL escaping/quoting, makes replica-identity failures easier to classify and remediate, and cleans up table state naming across core ETL and the Postgres-backed persistence layer.

Changes

Destination SQL quoting and escaping

  • Adds destination-specific SQL helper modules:
    • bigquery/sql.rs for GoogleSQL identifier/path quoting.
    • clickhouse/sql.rs for ClickHouse identifier quoting.
    • ducklake/sql.rs for DuckDB/DuckLake identifier quoting and lake catalog qualification.
    • snowflake/sql.rs for Snowflake identifiers and string literals.
    • sql.rs for shared ANSI double-quoted identifier escaping.
  • Moves BigQuery identifier escaping out of BigQueryClient and reuses it for fully qualified table paths, INFORMATION_SCHEMA.TABLES paths, column DDL, and primary key clauses.
  • Updates ClickHouse DDL/test utilities to use the new ClickHouse quoting helper, including create/drop database statements.
  • Updates DuckLake SQL generation to quote and qualify target table names consistently instead of string-replacing generated DDL.
  • Updates DuckLake mutation SQL to quote column names in delete predicates, update assignments, staging table operations, truncate, delete, update, and insert-select paths.
  • Keeps Postgres metadata SQL in DuckLake explicit by renaming that helper to quote_postgres_identifier, separating Postgres metadata SQL from DuckDB/DuckLake SQL.
  • Updates Snowflake table existence checks to use SHOW TERSE TABLES ... STARTS WITH ... plus exact row matching, with centralized string literal quoting.

Replica identity error classification

  • Adds ErrorKind::SourceReplicaIdentityError.
  • Reclassifies destination replica-identity incompatibilities from broader error kinds to the new source replica identity error kind across BigQuery, ClickHouse, DuckLake, Iceberg, and Snowflake.
  • Adds a specific manual remediation policy for replica-identity errors explaining how to choose the least costly supported replica identity:
    • REPLICA IDENTITY DEFAULT with a primary key.
    • REPLICA IDENTITY USING INDEX when supported.
    • REPLICA IDENTITY FULL only when full row images are needed.
  • Adds a BigQuery helper for partial update row errors so those failures also use the replica identity error kind.

Table state structure and naming

  • Flattens the core etl::state module so table-specific state lives directly under state/:
    • state/destination_table_metadata.rs.
    • state/table_error.rs.
    • state/table_retry_policy.rs.
    • state/table_state.rs.
  • Renames the core ETL domain types to state-oriented names:
    • TableState.
    • TableStateType.
    • TableError.
    • TableRetryPolicy.
  • Flattens the public state API so callers use etl::state::{TableState, TableStateType, TableError, TableRetryPolicy} instead of a nested table module.
  • Renames StateStore table APIs to state-oriented names:
    • get_table_state.
    • get_table_states.
    • load_table_states.
    • update_table_state.
    • update_table_states.
    • rollback_table_state.
  • Renames the cached table map alias from TableReplicationStates to TableStates.
  • Aligns ETL Postgres persistence modules with the same concepts:
    • destination_metadata -> destination_table_metadata.
    • state -> table_state.
  • Keeps the persisted Postgres representation clearly separate from the core domain model:
    • TableStateRow represents the database row.
    • StoredTableStateType represents the database enum value.
    • Core ETL code converts with TableState::to_storage_format() and TableState::from_state_row(...).
  • Keeps database-facing functions row/state-oriented (get_table_state_rows, update_table_state_raw, rollback_table_state, reset_table_state, delete_table_state(s)) while exposing core ETL state values to the rest of the pipeline.
  • Simplifies TableError so it stores replication failure details and the source error without carrying a table id.
  • Reuses the shared worker error handling policy when building table errors.
  • Adds Hash support and conversions for TableStateType.
  • Converts table state metrics from string-keyed counts to TableStateType-keyed counts, converting to the static metric label only at emission time.
  • Renames the table-state metric label from phase to state.
  • Updates table-state logs, comments, and public docs so they no longer refer to table replication phases.
  • Renames API schema/internal symbols from the old table-replication-state vocabulary to SimpleTableState, TableStatus, MissingTableState, and InvalidTableState.
  • Updates API rollback/status and replicator error reporting call sites to bridge from persisted TableStateRow metadata into core TableState values.

Migration and persistence notes

  • Leaves migration files unchanged relative to origin/main; the table-state rename is code-level and does not require a database migration.
  • Keeps the persisted table-state JSON representation on the same serde-tagged shape (type, lsn, reason, solution, and retry_policy).
  • Keeps source_err memory-only and skipped from persisted status JSON, so newly added or removed internal ErrorKind variants are not stored in table state.
  • Adds an AGENTS.md rule that migration files, including comments and non-executable text, must not be modified unless explicitly requested.

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 73.138% (+0.2%) from 72.926% — improve-code into main

@iambriccardo iambriccardo marked this pull request as ready for review May 27, 2026 11:53
@iambriccardo iambriccardo requested a review from a team as a code owner May 27, 2026 11:53

@farazdagi farazdagi 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.

Snowflake destination definitely looks nicer with that sql.rs extracted.

@farazdagi farazdagi enabled auto-merge (squash) May 27, 2026 12:12
@iambriccardo iambriccardo disabled auto-merge May 27, 2026 12:23
@iambriccardo iambriccardo merged commit 6a866ca into main May 27, 2026
16 checks passed
@iambriccardo iambriccardo deleted the improve-code branch May 27, 2026 12:51
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.

4 participants