Skip to content

Commit 5deba8e

Browse files
committed
Add regression tests for review findings; update plan docs
- SF golden MERGE SQL byte-identical to pre-refactor generator (fixture captured by running the merge-base generator); dotted per-component quoting + LegacyDotted toast-map keying - BQ raw-name filter pinned to LegacyDotted; CH legacy-normalized dotted destination BuildQuery test - Dotted CTID partitioning unit test (dotted schema/parent/children) - E2e: dotted partitioned-parent ctid QRep, initial-snapshot-only dotted mirror, add-tables union validation rejections, skip-validation structural check, source dotted collision - Snapshot clone workflow ID pinned to the legacy format; pua record.target/source legacy-dotted test; schema-helpers dotted overlap tests; Normalize*/Denormalize* helper round-trips - SF dotted e2e namespace suffixed to avoid cross-run collisions - Review findings doc + progress doc updated
1 parent 534b2e0 commit 5deba8e

12 files changed

Lines changed: 988 additions & 2 deletions

.claude/plans/qualified-table-identifiers-progress.md

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,9 @@ Branch: `qualified-table-identifiers`
8383
continued syncing through upgrade (raw-table LegacyDotted round-trip live)
8484
- [x] D: per-component units — identifiers, BQ convertToDatasetTable matrix, EventHub
8585
scoped test, CH dotted-destination BuildQuery (headline), SF per-component
86-
normalize, pua legacy-dotted
86+
normalize; pua legacy-dotted + SF golden MERGE + schema-helpers tests were
87+
MISSING (claimed done in error) — added during the post-implementation
88+
adversarial review (see review-findings doc)
8789

8890
## Verification (after implementation)
8991
- [x] build: go build ./... clean (flow + pkg modules); cargo check (nexus) clean;
@@ -102,6 +104,14 @@ Branch: `qualified-table-identifiers`
102104
## STATUS: COMPLETE (2026-06-12)
103105
All plan phases implemented and verified. 17 commits on qualified-table-identifiers.
104106

107+
## Adversarial review (2026-06-12)
108+
109+
An ultracode adversarial review of the finished branch found 14 distinct confirmed
110+
bugs (1 critical: ES dotted index rerouting; 3 high: CH dotted destination retarget,
111+
InitialLoadSummary namespace loss, add-tables collision validation) — all fixed, plus
112+
test gaps closed. Full findings, fixes, disprovals and the verification log:
113+
`.claude/plans/qualified-table-identifiers-review-findings.md`.
114+
105115
## Known acceptable gaps
106116
- Dotted-name resync e2e not written: resync `_resync`/`_peerdb_resync` suffix operates
107117
on `.Table` only (compile-checked struct logic), resync flows covered dotless by
@@ -111,6 +121,13 @@ All plan phases implemented and verified. 17 commits on qualified-table-identifi
111121
- EventHub names containing dots remain unsupported (documented; packing ambiguity,
112122
parity with pre-refactor).
113123
- Legacy strings in persisted configs to be dropped in release N+1 (follow-up).
124+
- Plan B.7 queue-destination dotted e2e (Kafka/PubSub/ES) not written — no local
125+
Kafka/PubSub/ES infra; ES dotted routing is covered by the LegacyDotted fix + review,
126+
kafka/pubsub already route via LegacyDotted. CI follow-up candidate.
127+
- Plan C.4 catalog-migration backfill Go tests replaced by manual verification on 30
128+
real catalog rows + SQL-vs-Go-normalizer audit (see review-findings L7/T5).
129+
- Plan B.3 dotted table REMOVAL / cancel_table_addition dotted variants and full B.5/B.6
130+
dotted QRep matrix (incremental, ctid parent, CH QRep, Mongo QRep) remain partial.
114131

115132
## Deviations from plan
116133
- Legacy proto fields annotated via comments instead of [deprecated = true] to avoid

.claude/plans/qualified-table-identifiers-review-findings.md

Lines changed: 262 additions & 0 deletions
Large diffs are not rendered by default.

flow/connectors/bigquery/merge_stmt_generator_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,15 @@
11
package connbigquery
22

33
import (
4+
"fmt"
45
"reflect"
6+
"strings"
57
"testing"
68

79
"github.com/PeerDB-io/peerdb/flow/connectors/utils"
810
"github.com/PeerDB-io/peerdb/flow/generated/protos"
11+
"github.com/PeerDB-io/peerdb/flow/pkg/common"
12+
"github.com/PeerDB-io/peerdb/flow/shared/types"
913
)
1014

1115
func TestGenerateUpdateStatement(t *testing.T) {
@@ -183,3 +187,31 @@ func TestGenerateUpdateStatement_WithUnchangedToastColsAndSoftDelete(t *testing.
183187
t.Errorf("Unexpected result. Expected: %v,\nbut got: %v", expected, result)
184188
}
185189
}
190+
191+
// the raw-table filter must use the legacy dotted destination name — the exact string
192+
// the sync side writes into _peerdb_destination_table_name — including for dotted
193+
// name components arriving first-dot-split from legacy configs
194+
func TestGenerateFlattenedCTERawNameFilterIsLegacyDotted(t *testing.T) {
195+
m := &mergeStmtGenerator{
196+
shortColumn: map[string]string{"id": "_c0"},
197+
rawDatasetTable: datasetTable{dataset: "ds", table: "_peerdb_raw_my_mirror"},
198+
mergeBatchId: 7,
199+
}
200+
schema := &protos.TableSchema{
201+
Columns: []*protos.FieldDescription{{Name: "id", Type: string(types.QValueKindInt64)}},
202+
}
203+
204+
for _, tc := range []struct {
205+
dstTable common.QualifiedTable
206+
rawName string
207+
}{
208+
{dstTable: common.QualifiedTable{Namespace: "ds", Table: "my_table"}, rawName: "ds.my_table"},
209+
{dstTable: common.QualifiedTable{Namespace: "sch.ema", Table: "ta.ble"}, rawName: "sch.ema.ta.ble"},
210+
} {
211+
cte := m.generateFlattenedCTE(tc.dstTable, schema)
212+
want := fmt.Sprintf("_peerdb_destination_table_name='%s'", tc.rawName)
213+
if !strings.Contains(cte, want) {
214+
t.Errorf("flattened CTE filter for %v should contain %q, got: %s", tc.dstTable, want, cte)
215+
}
216+
}
217+
}

flow/connectors/clickhouse/normalize_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -557,3 +557,42 @@ func TestGenerateCreateTableSQLForNormalizedTable(t *testing.T) {
557557
})
558558
}
559559
}
560+
561+
// a pre-QualifiedTable config with destination string "dst.table" arrives first-dot-split
562+
// as {Namespace: "dst", Table: "table"}; the physical ClickHouse table and the raw-table
563+
// lookup must still both resolve to the original single-part name "dst.table"
564+
func TestBuildQuery_LegacyNormalizedDottedDestination(t *testing.T) {
565+
ctx := t.Context()
566+
tableName := common.QualifiedTable{Namespace: "dst", Table: "table"}
567+
tableSchema := &protos.TableSchema{
568+
Columns: []*protos.FieldDescription{
569+
{Name: "id", Type: string(types.QValueKindInt64)},
570+
},
571+
}
572+
g := NewNormalizeQueryGenerator(
573+
tableName,
574+
map[common.QualifiedTable]*protos.TableSchema{tableName: tableSchema},
575+
[]*protos.TableMapping{
576+
{
577+
SourceTable: &protos.QualifiedTable{Namespace: "public", Table: "src"},
578+
DestinationTable: &protos.QualifiedTable{Namespace: tableName.Namespace, Table: tableName.Table},
579+
},
580+
},
581+
10,
582+
5,
583+
false,
584+
false,
585+
map[string]string{},
586+
"raw_my_table",
587+
nil,
588+
false,
589+
"",
590+
shared.InternalVersion_Latest,
591+
nil,
592+
)
593+
594+
query, err := g.BuildQuery(ctx)
595+
require.NoError(t, err)
596+
require.Contains(t, query, "INSERT INTO `dst.table`")
597+
require.Contains(t, query, "_peerdb_destination_table_name = 'dst.table'")
598+
}

flow/connectors/eventhub/scoped_eventhub_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ func TestNewScopedEventhub(t *testing.T) {
2222
require.Error(t, err, "missing namespace")
2323
_, err = NewScopedEventhub(common.QualifiedTable{Namespace: "ns", Table: "hub"})
2424
require.Error(t, err, "missing partition key column")
25+
_, err = NewScopedEventhub(common.QualifiedTable{Namespace: "ns", Table: "hub.pk.extra"})
26+
require.Error(t, err, "more than 3 parts was rejected before QualifiedTable too")
2527
}
2628

2729
// legacy 3-part dotted destinations from Lua scripts keep parsing as before

flow/connectors/postgres/qrep_partition_test.go

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -827,3 +827,82 @@ func prepareTestData(t *testing.T, pool *pgx.Conn, schema string, includeNulls b
827827

828828
return rowsCount
829829
}
830+
831+
// CTID partitioning of a partitioned parent whose schema, parent and child names all
832+
// contain dots: ChildTableRange.child_table must carry exact QualifiedTable structs
833+
// and every child must map back to a partition (plan 7.D dotted watermark/child case).
834+
func TestCTIDPartitioningOnDottedPartitionedTable(t *testing.T) {
835+
t.Parallel()
836+
_, conn, connStr := setupTestSchema(t)
837+
838+
schemaName := "par.t_" + strings.ToLower(common.RandomString(8))
839+
parent := common.QualifiedTable{Namespace: schemaName, Table: "pa.rent"}
840+
parentQuoted := parent.String()
841+
842+
_, err := conn.Exec(t.Context(), "CREATE SCHEMA "+common.QuoteIdentifier(schemaName))
843+
require.NoError(t, err)
844+
t.Cleanup(func() {
845+
_, _ = conn.Exec(t.Context(), fmt.Sprintf("DROP SCHEMA IF EXISTS %s CASCADE", common.QuoteIdentifier(schemaName)))
846+
})
847+
848+
_, err = conn.Exec(t.Context(), fmt.Sprintf(`
849+
CREATE TABLE %s (
850+
id SERIAL,
851+
partition_key INT NOT NULL,
852+
value TEXT,
853+
PRIMARY KEY (partition_key, id)
854+
) PARTITION BY RANGE (partition_key)
855+
`, parentQuoted))
856+
require.NoError(t, err)
857+
858+
rowsPerPartition := 25
859+
numChildTables := 3
860+
children := make([]common.QualifiedTable, numChildTables)
861+
for i := range numChildTables {
862+
children[i] = common.QualifiedTable{Namespace: schemaName, Table: fmt.Sprintf("chi.ld_%d", i)}
863+
_, err = conn.Exec(t.Context(), fmt.Sprintf(
864+
`CREATE TABLE %s PARTITION OF %s FOR VALUES FROM (%d) TO (%d)`,
865+
children[i].String(), parentQuoted, i*rowsPerPartition, (i+1)*rowsPerPartition))
866+
require.NoError(t, err)
867+
}
868+
869+
for i := range numChildTables * rowsPerPartition {
870+
_, err = conn.Exec(t.Context(), fmt.Sprintf(
871+
`INSERT INTO %s (partition_key, value) VALUES ($1, $2)`, parentQuoted),
872+
i, fmt.Sprintf("val_%d", i))
873+
require.NoError(t, err)
874+
}
875+
876+
c := &PostgresConnector{
877+
connStr: connStr,
878+
Config: &protos.PostgresConfig{},
879+
conn: conn,
880+
logger: log.NewStructuredLogger(slog.With(slog.String(string(shared.FlowNameKey), "testCTIDDotted"))),
881+
}
882+
partitions, err := getQRepPartitions(t, c, &protos.QRepConfig{
883+
FlowJobName: "test_ctid_dotted_partitioned",
884+
NumRowsPerPartition: 10,
885+
NumPartitionsOverride: 8,
886+
Query: fmt.Sprintf(`SELECT * FROM %s WHERE ctid BETWEEN {{.start}} AND {{.end}}`, parentQuoted),
887+
QualifiedWatermarkTable: internal.QualifiedTableProto(parent),
888+
WatermarkColumn: "ctid",
889+
}, nil)
890+
require.NoError(t, err)
891+
require.NotEmpty(t, partitions)
892+
893+
childTableCounts := make(map[common.QualifiedTable]int)
894+
for _, p := range partitions {
895+
require.Nil(t, p.Range)
896+
require.NotEmpty(t, p.ChildTableRanges)
897+
for _, ctr := range p.ChildTableRanges {
898+
require.NotNil(t, ctr.ChildTable)
899+
childTableCounts[internal.QualifiedTableFromProto(ctr.ChildTable)]++
900+
}
901+
}
902+
903+
require.Len(t, childTableCounts, numChildTables)
904+
for _, child := range children {
905+
require.Positive(t, childTableCounts[child],
906+
"child %s should appear in at least one partition", child)
907+
}
908+
}

flow/connectors/snowflake/merge_stmt_generator_test.go

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,13 @@ package connsnowflake
22

33
import (
44
"reflect"
5+
"strings"
56
"testing"
67

78
"github.com/PeerDB-io/peerdb/flow/connectors/utils"
89
"github.com/PeerDB-io/peerdb/flow/generated/protos"
10+
"github.com/PeerDB-io/peerdb/flow/pkg/common"
11+
"github.com/PeerDB-io/peerdb/flow/shared/types"
912
)
1013

1114
func TestGenerateUpdateStatement(t *testing.T) {
@@ -148,3 +151,81 @@ func TestGenerateUpdateStatement_WithUnchangedToastColsAndSoftDelete(t *testing.
148151
t.Errorf("Unexpected result. Expected: %v, but got: %v", expected, result)
149152
}
150153
}
154+
155+
// goldenDotlessMergeStmt is the exact statement the pre-QualifiedTable generator
156+
// (merge-base 0709374b) produced for destination "public.myTable" with the schema in
157+
// TestGenerateMergeStmtGolden; the refactored generator must reproduce it byte-for-byte
158+
// so existing mirrors see unchanged SQL.
159+
//
160+
//nolint:lll
161+
const goldenDotlessMergeStmt = `MERGE INTO "PUBLIC"."myTable" TARGET USING (WITH VARIANT_CONVERTED AS (
162+
SELECT _PEERDB_UID,_PEERDB_TIMESTAMP,TO_VARIANT(PARSE_JSON(_PEERDB_DATA)) VAR_COLS,_PEERDB_RECORD_TYPE,
163+
_PEERDB_MATCH_DATA,_PEERDB_BATCH_ID,_PEERDB_UNCHANGED_TOAST_COLUMNS
164+
FROM _PEERDB_INTERNAL._PEERDB_RAW_my_mirror WHERE _PEERDB_BATCH_ID = 42 AND
165+
_PEERDB_DATA != '' AND
166+
_PEERDB_DESTINATION_TABLE_NAME = ? ), FLATTENED AS
167+
(SELECT _PEERDB_UID,_PEERDB_TIMESTAMP,_PEERDB_RECORD_TYPE,_PEERDB_MATCH_DATA,_PEERDB_BATCH_ID,
168+
_PEERDB_UNCHANGED_TOAST_COLUMNS,CAST(VAR_COLS:"id" AS INTEGER) AS "ID",CAST(VAR_COLS:"myData" AS STRING) AS "myData"
169+
FROM VARIANT_CONVERTED), DEDUPLICATED_FLATTENED AS (SELECT _PEERDB_RANKED.* FROM
170+
(SELECT RANK() OVER
171+
(PARTITION BY ("ID") ORDER BY _PEERDB_TIMESTAMP DESC) AS _PEERDB_RANK, * FROM FLATTENED)
172+
_PEERDB_RANKED WHERE _PEERDB_RANK = 1)
173+
SELECT * FROM DEDUPLICATED_FLATTENED) SOURCE ON TARGET."ID" = SOURCE."ID"
174+
WHEN NOT MATCHED AND (SOURCE._PEERDB_RECORD_TYPE != 2) THEN INSERT ("ID","myData","_PEERDB_SYNCED_AT") VALUES(SOURCE."ID",SOURCE."myData",CURRENT_TIMESTAMP)
175+
WHEN MATCHED AND
176+
(SOURCE._PEERDB_RECORD_TYPE != 2) AND _PEERDB_UNCHANGED_TOAST_COLUMNS=''
177+
THEN UPDATE SET "ID" = SOURCE."ID", "myData" = SOURCE."myData", "_PEERDB_SYNCED_AT" = CURRENT_TIMESTAMP, "_PEERDB_IS_DELETED" = FALSE WHEN MATCHED AND
178+
(SOURCE._PEERDB_RECORD_TYPE = 2) AND _PEERDB_UNCHANGED_TOAST_COLUMNS=''
179+
THEN UPDATE SET "ID" = SOURCE."ID", "myData" = SOURCE."myData", "_PEERDB_SYNCED_AT" = CURRENT_TIMESTAMP, "_PEERDB_IS_DELETED" = TRUE WHEN NOT MATCHED AND (SOURCE._PEERDB_RECORD_TYPE = 2) THEN INSERT ("ID","myData","_PEERDB_SYNCED_AT",_PEERDB_IS_DELETED) VALUES(SOURCE."ID",SOURCE."myData",CURRENT_TIMESTAMP,TRUE)
180+
WHEN MATCHED AND (SOURCE._PEERDB_RECORD_TYPE = 2) THEN UPDATE SET _PEERDB_IS_DELETED = TRUE, _PEERDB_SYNCED_AT = CURRENT_TIMESTAMP`
181+
182+
func TestGenerateMergeStmtGolden(t *testing.T) {
183+
schema := &protos.TableSchema{
184+
Columns: []*protos.FieldDescription{
185+
{Name: "id", Type: string(types.QValueKindInt64)},
186+
{Name: "myData", Type: string(types.QValueKindString)},
187+
},
188+
PrimaryKeyColumns: []string{"id"},
189+
}
190+
191+
newGenerator := func(dstTable common.QualifiedTable, rawName string) *mergeStmtGenerator {
192+
return &mergeStmtGenerator{
193+
tableSchemaMapping: map[common.QualifiedTable]*protos.TableSchema{dstTable: schema},
194+
unchangedToastColumnsMap: map[string][]string{rawName: {""}},
195+
peerdbCols: &protos.PeerDBColumns{
196+
SyncedAtColName: "_PEERDB_SYNCED_AT",
197+
SoftDeleteColName: "_PEERDB_IS_DELETED",
198+
},
199+
rawTableName: "_PEERDB_RAW_my_mirror",
200+
mergeBatchId: 42,
201+
}
202+
}
203+
204+
t.Run("dotless byte-identical to pre-refactor", func(t *testing.T) {
205+
dstTable := common.QualifiedTable{Namespace: "public", Table: "myTable"}
206+
stmt, err := newGenerator(dstTable, "public.myTable").generateMergeStmt(t.Context(), nil, dstTable)
207+
if err != nil {
208+
t.Fatal(err)
209+
}
210+
if stmt != goldenDotlessMergeStmt {
211+
t.Errorf("merge statement diverged from pre-QualifiedTable output:\ngot:\n%s\nwant:\n%s", stmt, goldenDotlessMergeStmt)
212+
}
213+
})
214+
215+
t.Run("dotted components quoted per-component", func(t *testing.T) {
216+
dstTable := common.QualifiedTable{Namespace: "sch.ema", Table: "ta.ble"}
217+
stmt, err := newGenerator(dstTable, "sch.ema.ta.ble").generateMergeStmt(t.Context(), nil, dstTable)
218+
if err != nil {
219+
t.Fatal(err)
220+
}
221+
if !strings.HasPrefix(stmt, "MERGE INTO \"SCH.EMA\".\"TA.BLE\" TARGET") {
222+
t.Errorf("dotted destination not quoted per-component: %s", stmt[:80])
223+
}
224+
// the unchanged-toast map is keyed by the raw-table string (LegacyDotted of the
225+
// destination); a quoted-String() or .Table key would miss and silently drop
226+
// the toast-handling UPDATE branch from the statement
227+
if !strings.Contains(stmt, "_PEERDB_UNCHANGED_TOAST_COLUMNS=''") {
228+
t.Error("LegacyDotted-keyed unchangedToastColumnsMap lookup missed; toast UPDATE branch absent")
229+
}
230+
})
231+
}

0 commit comments

Comments
 (0)