fix(clickhouse): preserve legacy soft-delete column casing

[floze-the-genius]

Summary

• resolve the configured soft-delete column against the physical ClickHouse destination schema before normalization
• prefer exact matches, then case-insensitive matches, so both legacy lowercase tables and newer uppercase/custom tables keep working
• apply the same resolution during destination validation

Root cause

Older PeerDB releases stored the UI default _PEERDB_IS_DELETED in mirror config, but ClickHouse table creation and normalization hardcoded the physical column as _peerdb_is_deleted. After #4365 began honoring the stored config, upgraded legacy mirrors generated inserts for the uppercase identifier even though their existing tables only had the lowercase column.

Tests

• go test ./connectors/clickhouse -skip '^TestCreateRawTableHasTTL$' -count=1
• go vet ./connectors/clickhouse
• go test -race ./connectors/clickhouse -run 'Test(ResolveSoftDeleteColumnName|BuildQuery_UsesDestinationSoftDeleteColumnCase|ProcessTableComparison_WithResolvedSoftDeleteColumnCase)$' -count=1

Fixes #4596

[CLAassistant]

All committers have signed the CLA.

[pfcoperez]

Thanks for your contribution! I've dropped a comment about the implementation detail.

[floze-the-genius]

Narrowed the compatibility behavior in ac5fe774 as requested:

• exact destination-column matches still win;
• fallback resolution is now limited to the two known equivalent defaults, _PEERDB_IS_DELETED and _peerdb_is_deleted;
• mixed-case variants and custom column names remain strict, so my_deleted will not resolve to MY_deleted.

Added positive tests for both legacy directions and negative tests for mixed/default and custom-case variants. Targeted ClickHouse tests and go vet ./connectors/clickhouse pass.

[pfcoperez]

@fable review this PR

[claude]

Claude finished @pfcoperez's task in 3m 56s —— View job

---

Code Review

• Gather context (diff, changed files, review guidelines)
• Review normalize.go resolution logic
• Review validate.go changes
• Review tests
• Check other call sites of the soft-delete column for consistency
• Post review feedback

---

Verdict

The fix is correct and well-scoped. The narrowed behavior requested in the earlier review round is implemented as described: an exact destination-column match always wins, the fallback is strictly limited to the _PEERDB_IS_DELETED ⇄ _peerdb_is_deleted pair, and custom/mixed-case names stay strict (my_deleted will not resolve to MY_deleted). No blocking issues found — a couple of minor suggestions below.

What I verified

• Resolution logic (normalize.go:650-677): exact match first; the two-defaults fallback only triggers when the configured name is one of the two known defaults; when a table has both variants, exact match wins (covered by the first test case). When the table is missing from the columns map (not yet created, or excluded), it falls back to the configured name — identical to pre-PR behavior, so no regression on that path.
• Per-table resolution is the right granularity: a legacy mirror can have a mix of old lowercase tables and newer tables added after the upgrade.
• Cluster mode: the normalize INSERT targets t.TableName (normalize_query.go:369-370), which is exactly the name whose columns are fetched via system.columns WHERE database=currentDatabase() — consistent.
• Completeness: _peerdb_synced_at doesn't have the analogous bug because SyncedAtColName is consistently lowercased at both table creation (normalize.go:229) and validation (validate.go:138); soft-delete was the only column with the creation/config divergence. Table creation (generateCreateTableSQLForNormalizedTable) keeps using the configured name, which is correct — new and resynced tables then match exactly at normalize time.
• Validation (validate.go:133-139): moving tablePeerDBColumns inside the loop is necessary since resolution is now per-table; versionColName and synced-at handling are unchanged.
• REVIEW.md checks: no new stdout/log output containing sensitive data; not a dependency bump.

Minor suggestions (non-blocking)

1. Skip the system.columns lookup when resolution can't change anything — normalize.go:468-473. For a custom soft-delete column name (anything other than ""/_peerdb_is_deleted/_PEERDB_IS_DELETED), resolveSoftDeleteColumnName is always the identity, yet every NormalizeRecords cycle now pays an extra system.columns round-trip and gains a new hard-failure point. Gating the fetch on the configured name being one of the two defaults (or empty) would keep the fix free for custom-named mirrors. Fix this →
2. Redundant empty-name defaulting — validate.go:54-57 maps "" → _peerdb_is_deleted before calling resolveSoftDeleteColumnName, which already does that itself (normalize.go:654-656). You could pass cfg.SoftDeleteColName straight through and drop those lines.

Tests / CI

The new tests cover both legacy directions, the both-columns-present case, and the negative mixed-case/custom-name cases; TestBuildQuery_UsesDestinationSoftDeleteColumnCase verifies the resolved name actually lands in the generated INSERT. I couldn't run go test/go vet in this sandbox (commands not permitted), but GolangCI-Lint passed on the branch and "Flow build and test" is awaiting maintainer approval to run; the author reports the targeted tests pass locally.

[pfcoperez]

👍

[pfcoperez]

1. For a custom soft-delete column name (anything other than ""/_peerdb_is_deleted/_PEERDB_IS_DELETED), resolveSoftDeleteColumnName is always the identity, yet every NormalizeRecords cycle now pays an extra system.columns round-trip and gains a new hard-failure point. Gating the fetch on the configured name being one of the two defaults (or empty) would keep the fix free for custom-named mirrors.

This is a good point @floze-the-genius and it seems a simple enough change.

Besides that, the PR looks good to me, maybe @ilidemi or @jgao54 want to take a look too.

[jgao54]

this introduces a clickhouse query per table table on every normalize iteration, and the destination column name does not change from iteration to iteration. so this would add latency to all pipes.

the history around upper case _peerdb_is_deleted is a bit unfortunate. initially, the SoftDeleteColName field was originally never used by clickhouse, it was only for snowflake/bigquery/postgres destinations. then #2008 started to use SoftDeleteColName for ClickHouse to support the delete-on-merge feature, which accidentally made it possible for CH to also have upper case _peerdb_is_deleted. while this bug was fixed immediately in #3574, it was only fixed in the UI path so it's still technically possible in the API path (and still possible for legacy existing pipes that have the config set to upper case)

my preference is actually to revert change introduced in #4365 and also tighten the validation on the server-side to prevent upper case from being introduced.

@ilidemi wdyt?

[ilidemi]

Agreed. I don't have as much context about this feature, so if you think tightening validation is going to remove degrees of freedom in a good way, that sounds easier.

Does this mean that the existing setups would be recommended to rename the CH column, or would we keep some kind of handling to accommodate for both?