Repository navigation
fix(writer): bind source NULLs, same-version relocation tombstones, keyless ORDER BY, event-identity dedup, TIME range, missing target columns, UInt64 - #1474
Merged
minguyen9988 merged 7 commits intoSep 20, 2026
Conversation
…pe fixes before the code Spec-first declaration (Smart Ralph) for seven defects in the write path: - 07.07 / 04.03: the bind path reads the STORED value with Struct.getWithoutDefault; Struct.get returns the Connect-schema default (the MySQL column DEFAULT via Debezium) for a source NULL, so NULLs in any column with a MySQL default were written as the default. non.default.value is deprecated and a no-op. - 05.02 / Constitution I4: the relocation tombstone carries the record's own _version, not V-1. Under GTID versioning an INSERT and a relocating UPDATE in one transaction share a version, so a V-1 tombstone lost the merge and left a ghost row; the equal case is decided by the ReplacingMergeTree later-insert-wins tie rule, which the tombstone wins. - 08.05: an auto-created table with no usable primary key gets ORDER BY over every source column (allow_nullable_key=1 when any is Nullable) and the keyless banner; ORDER BY tuple() collapsed the whole table to one row. The engine clause text is corrected to what the code emits. - 10.05 (new): deduplication.policy keys on event identity (topic, partition, offset), never on the row key, and the pool is bounded by buffer.count on the map actually consulted. - 07.03: MySQL TIME is a signed duration (-838:59:59..838:59:59); MicroTime is formatted from the signed total, never reduced modulo 24h; target is String. - 08.04 / 08.03: a source column absent from ClickHouse is added (schema.evolution=true) or fails the batch with MissingTargetColumnException; only ALIAS is ever proven-absent. - 02.05: names the real check (PreparedStatementFieldMapper .rejectUnderivableVersion, IllegalStateException) and rejects _version <= 0. - 07.01: BIGINT UNSIGNED arrives as INT64 under Debezium's default long mode; values >= 2^63 wrap negative and are restored to their unsigned magnitude for a UInt64 target. - 11.03 / Lean README: tombstone shares the live version; tie rule stated; lake build needs a default target. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…R BY, event-identity dedup, TIME range, missing columns, UInt64 Seven write-path defects, each with a regression test that fails without the fix (mutation-checked: fix reverted -> red, restored -> green). 1. Source NULL replaced by the Connect-schema default (silent divergence). PreparedStatementFieldMapper bound struct.get(col), which returns schema.defaultValue() for a null field; Debezium fills that from the MySQL column DEFAULT, so NULL in any column with a MySQL default was written as 'new' / 0 / 1970-01-01 with matching row counts. The default-free read was gated behind non.default.value=true, whose default was false. Now getWithoutDefault is used unconditionally at both bind sites and in ClickHouseStruct's modified-field lists; non.default.value is a deprecated no-op that warns once when set to false. Test: NullValueColumnDropTest.testSchemaDefaultIsNotSubstitutedForNull (red: 'Bound strings: [new]'). 2. Ghost row after a sorting-key relocation. The tombstone was written at _version - 1. Under GTID versioning every row event of a transaction shares one version, so INSERT k='a' then UPDATE ... k='b' in one transaction left tombstone(V-1) < live(V): FINAL kept 'a' AND 'b' (reproduced with clickhouse local). The tombstone now binds record.getVersion(); the equal-version case resolves to the later insert. Test: PreparedStatementFieldMapperTombstoneVersionTest (red: expected V, was V-1). 3. Keyless table collapses to one row. ClickHouseAutoCreateTable emitted ORDER BY tuple() on ReplacingMergeTree when the record carried no primary key; two distinct rows in, one out. The sorting key is now every source column (connector columns excluded), SETTINGS allow_nullable_key=1 is appended when any key column is Nullable (merged with user settings), and KeylessTableWarning is logged. Never tuple(). Test: ClickHouseAutoCreateTableTest.testKeylessTableOrdersByAllColumns and three siblings. 4. deduplication.policy=OLD|NEW dropped every UPDATE/DELETE. DeDuplicator keyed on record.key() (the row PK), and for an existing topic returned 'not new' for every unseen key too; it pruned an always-empty queue so the map consulted grew without bound. The key is now (topic, partition, offset); pruning evicts from the consulted map. Test: DeDuplicatorTest (red: 'it holds 25 identities'). 5. MySQL TIME wrapped modulo 24h. MicroTimeConverter went through LocalTime, so -01:00:00 became 23:00:00 and 25:30:00 became 01:30:00. It now formats the signed microsecond total (unbounded hours). Test: DebeziumConverterTest.testMicroTimeConverterSignedAndBeyond24Hours. 6. Missing plain column silently dropped forever. When a source column was absent from ClickHouse and neither ALIAS nor MATERIALIZED, the writer logged once, marked it proven-absent and omitted the value on every later record. Now: schema.evolution=true -> ALTER TABLE ... ADD COLUMN via the existing ClickHouseAlterTable path and the same batch binds it; otherwise MissingTargetColumnException names table, column and the knob. Only ALIAS is proven-absent. Test: GroupInsertQueryWithBatchRecordsTest (4 cases). 7. Version validation and BIGINT UNSIGNED. rejectUnderivableVersion now rejects _version <= 0 (0 is producible only by a corrupt coordinate). A BIGINT UNSIGNED >= 2^63 arrives as a negative long under Debezium's default long mode; for a UInt64 target it is bound as its unsigned BigInteger magnitude. Tests: RejectUnderivableVersionTest, ClickHouseDataTypeMapperUInt64Test. sink-connector module: 430 tests, 0 failures, 20 errors -- all 20 are pre-existing testcontainers 'Could not find a valid Docker environment' errors in DBMetadataTest, DbWriterTest, DbKafkaOffsetWriterTest and ClickHouseCreateDatabaseTest, unrelated to this change. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t-wins tie; make lake build real
Engine.lean: tombstoneVersion i = liveVersion i (was 2*i - 1), matching the
connector binding record.getVersion() for both rows of a relocation.
Upgrade.lean: translateEventAtV tombstone is v i (was v i - 1); GapMono is
kept unchanged so the I11 statement is unchanged.
Proofs.lean: replicate_converges_gen's relocation case is re-proved without
a strict tombstone/live ordering, using the new chFinalView_snoc_other
(a record of another key never changes the view). New theorems:
- findMaxVersion_snoc_ge: a record whose version is >= every earlier one
is the maximum (ties go to the later record, as maxStep's >= says).
- tombstone_wins_version_tie: a tombstone appended after a live row of the
same key and the SAME version evaluates the key to none.
- decremented_tombstone_loses: the V-1 tombstone leaves the live row
visible -- the ghost row.
update_pk_relocation_soundness, master_replication_convergence, upgrade_safe
and the single-event corollaries all re-check; every theorem depends only on
[propext, Quot.sound]; zero sorry.
lakefile.lean: the lean_lib is now @[default_target]. Without it a bare
'lake build' -- the command every spec cites -- had no target, printed
'Build completed successfully' and compiled nothing.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… writable
Review finding on the missing-column change: the MATERIALIZED branch of
GroupInsertQueryWithBatchRecords.refreshIfRecordHasUnknownColumn still
swallowed an enforcement failure. When enforceSourceColumnIsWritable
returned false (declared type or expression unreadable, ALTER rejected or
without effect) or the post-DDL re-read still lacked the column, it called
markColumnProvenAbsent and returned the stale map -- so the INSERT was built
without the source value, ClickHouse kept its computed value, and the offset
advanced past the row. That is the same silent-divergence class this branch
eliminates for absent columns, and per the prime directive the MATERIALIZED
definition is what is wrong; the connector must not continue without the
source value.
Now that path throws MissingTargetColumnException naming the database,
table, column and the remediation that failed (ALTER TABLE ... MODIFY COLUMN
... DEFAULT <expression>, or granting ALTER TABLE). markColumnProvenAbsent
is reserved strictly for ALIAS columns.
Spec 08.04 sections 3.1 and 3.3 and spec 08.03 section 3.2 updated first.
Tests (GroupInsertQueryWithBatchRecordsTest, in-memory system.columns fake):
- materializedColumnWhoseConversionFailsFailsBatch: type unreadable ->
enforcement false -> exception, no proven-absent entry, no DDL issued.
- materializedColumnStillMissingAfterConversionFailsBatch: MODIFY COLUMN
reads back DEFAULT but the re-read map still lacks the column ->
exception, no proven-absent entry.
- materializedColumnConvertedIsBoundInSameBatch: the successful conversion
binds the column in the same batch (guards the happy path).
Both failure tests are red with the previous swallow reinstated
('Expected MissingTargetColumnException to be thrown, but nothing was
thrown') and green with the fix; 7/7 in the class, 17/17
UnwritableColumnReportingTest, 6/6 NullValueColumnDropTest.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rder-by Forward-merge of the base branch. The lakefile conflict kept both sides: the default_target attribute from the base and the explanatory comment from this branch. The destructive-operation sites reported for this merge are all inherited from the base branch: DDL parser test fixtures asserting the translated DROP TABLE IF EXISTS / DROP COLUMN IF EXISTS statements, and the Lean DdlTranslation model constructor for DROP COLUMN. None of them executes anything; no destructive code is introduced by this merge. Destructive-Op-Check: sites=11; result=pass
…s fixture table DbWriterTest.getSampleRecords() fed records with columns quantity, amount and employed into GroupInsertQueryWithBatchRecords.groupQueryWithRecords() against the real employees fixture table, which has none of them. The pre-fix writer silently dropped such columns from the INSERT and marked them proven-absent; under Spec 08.04 section 3.3 a source column that does not exist in ClickHouse now fails the batch with MissingTargetColumnException unless schema.evolution adds it, so DbWriterTest.testGroupRecords and DbKafkaOffsetWriterTest.testInsertTopicOffsetMetadata (which reuses the same records) errored in the java-kafka CI job, where the Testcontainers ClickHouse is available. Those two tests exercise offset bookkeeping, not the missing-column rule (covered by GroupInsertQueryWithBatchRecordsTest), so the sample row now mirrors the fixture table: emp_no, birth_date, first_name, last_name, gender, hire_date. Verified locally that the records group into a single INSERT INTO employees(emp_no,birth_date,first_name,last_name,gender,hire_date,_version) template and yield the asserted per-partition offsets.
…gressIT Spec 01.06 section 3.1 moved the deliberate skip of a heartbeat or transaction-boundary record from WARN to DEBUG (a WARN per heartbeat buried the warnings that matter); only a row record the parser cannot convert still warns. UnparseableRecordProgressIT still looked for a WARN containing 'skipping' to prove the unparseable path was reached, so on the current base it found nothing and failed with 'no unparseable record reached the connector during this run' in every java-lightweight job, including this PR's. The test now opens the DebeziumChangeEventCapture logger to DEBUG for the run (restored afterwards, the same pattern ControlRecordLogLevelTest uses) and accepts either the DEBUG control-record line or the WARN row-record skip as proof that the path was exercised. The regression assertions are unchanged: no NullPointerException may be raised and every row inserted after the control records must reach ClickHouse.
minguyen9988
deleted the
fix-writer-source-null-tombstone-keyless-order-by
branch
September 20, 2026 10:52
minguyen9988
added a commit
that referenced
this pull request
Sep 20, 2026
…ns, retire the allowlist Merge origin/2.11.0 (writer fixes, #1474) into the governance branch. Conflicts (specs only), resolved to what the merged code does: - 08.03 proven-absent protocol: only an ALIAS column is ever proven absent (#1474) AND the proof is stamped with the table's current version and discarded once invalidateTable/invalidateAll bumps it (this branch); CacheInvalidationManager implements both. - 11.03 model design: base's same-version tombstone and FINAL tie-rule text, plus this branch's note that the shipped version formula, floor, seeds and GTID precedence are not modelled; 3.2 keeps the ReplayIdempotency note and the CI-gate heading with base's default_target explanation of lake build. Validator on the merged tree: every concurrent rewrite the allowlist waivers were parked behind has now landed, so the seven remaining allowlisted findings are fixed at the source and the allowlist carries no entries: - 04.03: QueryFormatter path is db/ (not db/operations/); there is no createColumns(); cites NullValueColumnDropTest and NullColumnValueRoundTripIT instead of tests that never existed. - 06.02: cites PauseDrainAtomicityTest / PauseDrainRaceTest instead of the non-existent ClickHouseBatchExecutorTest. - 06.03: MySQLDDLParserService.java (case). - 07.01: cites ClickHouseDataTypeMapperTest.getClickHouseDataType / getUnsignedClickHouseType and ClickHouseDataTypeMapperInt8Test. - 07.03: cites the DebeziumConverterTest boundary round-trips. - Pruned the five waivers the validator reported as no longer needed (02.05, 05.02, 07.07, 08.04, 08.05). Local verification: validate_specs.py PASSED (59 specs, 309 citations, 0 warnings); scripts/tests 39 passed; lake build 12 modules, no sorry / admit / native_decide; DebeziumChangeEventCaptureTest 23/23, SequenceSeedOverflowTest 3/3, DdlDrainDeadlockTest 6/6, OffsetHandoffOrderTest 6/6, MySqlDDLParserListenerImplTest 135/135, NullValueColumnDropTest 6/6.
minguyen9988
added a commit
that referenced
this pull request
Sep 22, 2026
… KEY changes loud, unsigned/SERIAL/FLOAT(M,D), spatial, CHECK, back-fill defaults, NOT NULL on MODIFY (#1479) * fix(ddl): key a keyless CREATE TABLE by every stored column on the DDL path, never ORDER BY tuple() A source table with no PRIMARY KEY and no fully NOT NULL UNIQUE key was still created by the DDL translator as ReplacingMergeTree ... ORDER BY tuple(). Under an empty sorting key every row compares equal, so the table collapses to ONE row -- total, silent data loss. The record-schema auto-create path stopped doing that in #1474 (all-columns fallback key), so the same source table got a different, row-losing identity depending on which path created it first. The comment in enterColumnCreateTable claimed the keyless preflight refuses such tables; it never did (it only warns). Now, on the DDL path (Spec 06.05 s3.6, mirroring Spec 08.05 s3.2): - ORDER BY (every non-generated column, in declaration order) when neither a PRIMARY KEY nor a NOT NULL UNIQUE key exists; the schema-override primary_key still wins and suppresses the fallback entirely. - nullableSortingKey is set when that key names a nullable column, so the existing (previously dead) SETTINGS allow_nullable_key=1 block is live and the CREATE is not rejected with Code: 44. - AUTO_INCREMENT implies NOT NULL (CREATE columns and ADD COLUMN), so `id INT AUTO_INCREMENT UNIQUE` is adopted as the identity. - KeylessTableWarning banner at ERROR plus a WARN naming the key. - A CREATE with no stored column at all is refused with DDLReplicationException instead of ORDER BY tuple(). - Constants.ORDER_BY_TUPLE removed (no consumer left). Tests flipped deliberately because they pinned the defect (ORDER BY tuple() or "no invented key"): MySqlDDLParserListenerImplTest.testCreateTable, testCreateTableWithNulLFields, testCreateTableWithSetDataType, testCreateTableWithKeyPartition, testCreateTableWithRangeByColumnsPartition, testCreateTableWithParitionRange (keyless half), testSourceWithIsDeletedColumn, testGeneratedColumn, testReplicatedReplacingMergeTreeWithoutIsDeletedColumn, checkIfTimestampDataTypePrecisionIsMaintained (+UpperCase); CreateTableNoKeySortKeyTest.testKeylessTableGetsNoInventedSortKey -> testKeylessTableGetsAllColumnsSortKey, testMultiColumnKeylessTableGetsNoAllColumnSortKey -> testMultiColumnKeylessTableGetsAllColumnSortKey; CreateTableUniqueKeySortKeyTest.testNoKeyAtAllHasNoInventedSortKey -> testNoKeyAtAllGetsAllColumnsSortKey, testNullableUniqueKeyFallsBackToAllColumns, testPartiallyNullableCompositeUniqueKeyFallsBackToAllColumns (now assert the fallback key and the setting). New: testCreateTableKeylessOrdersByAllColumns, testAutoIncrementColumnIsNotNull. Formal: new Replication.CreateTable (sorting_key_nonempty, primary_key_wins, unique_key_wins_when_not_null, fallback_key_is_every_stored_column, declared_key_never_needs_nullable_setting, nullable_fallback_gets_setting), registered in the lakefile, the Lean README and CONSTITUTION s5. * fix(ddl): map every unsigned integer spelling, SERIAL and FLOAT(M,D) to types that hold the source values DataTypeConverter matched the resolver's type name -- which carries the attribute tokens verbatim -- exactly against six spellings, so `BIGINT UNSIGNED ZEROFILL`, `INT8 UNSIGNED`, `INT(10) UNSIGNED ZEROFILL`, `INT ZEROFILL` and `SERIAL` were all created SIGNED (Int64 / Int32), and `INT1` became Int16. Values above the signed maximum cannot be stored in such a column. `FLOAT(7,3)` copied its dimensions into `Float64(7,3)`, which ClickHouse rejects, so the CREATE/ALTER failed on every retry. Now (Spec 07.01 s3.2, Spec 07.02 s3.1): - normalizeIntegerTypeName folds INT1/INT2/INT3/MIDDLEINT/INT4/INT8 and SERIAL, applies MySQL's rule that ZEROFILL implies UNSIGNED, drops ZEROFILL/SIGNED, and every unsigned spelling resolves through ClickHouseDataTypeMapper.getUnsignedClickHouseType -- the same function the record path uses. - SERIAL is UInt64 NOT NULL on CREATE and ADD COLUMN, and the candidate UNIQUE key on CREATE. - A (precision[, scale]) suffix is emitted only for Decimal and DateTime64; FLOAT(M,D), FLOAT and DOUBLE(M,D) are Float64, REAL stays Float32. - Spec 07.02 s3.1 no longer claims FLOAT -> Float32 on the DDL path (it never emitted that); the DDL-path mapping is now written down. Tests: testUnsignedSynonymsAndZerofillMapToUInt, testSerialIsUnsignedNotNullKey, testFloatWithDimensionsIsFloat64 (all red before the fix). * fix(ddl): map the MySQL spatial family to String (WKB hex) on CREATE and ALTER POINT and POLYGON were emitted as the ClickHouse Geo types: CREATE forced them NOT NULL (the Geo types cannot be Nullable), ALTER ADD COLUMN of a nullable geometry emitted Nullable(Polygon)/Nullable(Point), which ClickHouse rejects, and every non-point kind (LINESTRING, MULTI*, GEOMETRYCOLLECTION) was mapped to Polygon. A rejected DDL is retried, so the stream stalled; a column that cannot be NULL cannot hold a nullable source column. Now every spatial type maps to String on every DDL path (CREATE, ADD, MODIFY, CHANGE), Nullable exactly when the source column is, so the column holds the source WKB bytes as hex byte-for-byte (Spec 07.06). JSON, parsed by the same grammar alternative, keeps its String mapping. The value-path half (binding the WKB as hex) is the companion change on the writer side. Constants.NULLABLE_NOT_SUPPORTED_DATA_TYPES (the Point/Polygon exemption) is removed with its only consumer. Test: testAlterAddNullableGeometryIsRepresentable (red before the fix: Nullable(Polygon) / Point). * fix(ddl): skip CHECK constraints (ADD CHECK / DROP CONSTRAINT) instead of echoing them ADD [CONSTRAINT k] CHECK (expr) was echoed verbatim into the ClickHouse ALTER and DROP CONSTRAINT k became DROP CONSTRAINT IF EXISTS k. An unnamed CHECK, a NOT ENFORCED clause or any MySQL-only function in the expression is rejected by ClickHouse; because DDL is retried, the statement -- and every ADD COLUMN riding in it -- stalled the stream. When ClickHouse did accept the CHECK it enforced it on INSERT against rows MySQL had already validated (possibly under a later-relaxed or NOT ENFORCED constraint), failing batches. CREATE TABLE already dropped CHECKs, so ALTER disagreed with it. Both alterByAddCheckTableConstraint and alterByDropConstraintCheck are now in the skip class (Spec 06.03 s3.2, Spec 06.04 s3.1/s3.3): nothing is emitted for the clause, neighbours are kept, an all-CHECK statement translates to "". Constants.DROP_CONSTRAINT and the parseTreeHelper echo are removed with their only consumers. Tests flipped deliberately because they pinned the echo: testDropContraints, testAddConstraintsWithAnd, testAlterTableAddConstraint. New: testAddCheckConstraintIsSkipped (red before the fix). * fix(ddl): make the ADD COLUMN back-fill match MySQL and rewrite literal DEFAULTs to ClickHouse syntax A ClickHouse DEFAULT only ever decides what the rows that pre-date an ADD COLUMN hold, but that is exactly where the translator diverged from MySQL, count-clean and permanently: - `DEFAULT CURRENT_TIMESTAMP`/`NOW()` was dropped, so pre-existing rows held 1970-01-01 where MySQL back-filled the ALTER's own timestamp; - `ENUM(...) NOT NULL` without a DEFAULT got '' where MySQL back-filled the first member (its implicit default); - any other expression default (`(UUID())`, `(CURRENT_DATE)`, `(1 + 2)`) was dropped silently, leaving the type's zero value; - `b'0101'`, `X'0A0B'`/`0x0A0B` and double-quoted strings were copied verbatim, which ClickHouse rejects, so the ALTER failed on every retry. Now (Spec 06.04 s3.2): - literal defaults are rewritten: double-quoted -> single-quoted (escapes fixed, N'' and charset introducers dropped); bit-string and hex literals become the integer for numeric columns and, for String columns, the same BYTES representation the writer stores (lowercase hex text, or unhex(..) under persist.raw.bytes=true); - ADD COLUMN ... DEFAULT CURRENT_TIMESTAMP[(n)]/NOW()/LOCALTIME[STAMP] emits toDateTime64('<UTC wall clock>', p, 'UTC') / toDateTime(.., 'UTC') derived from the DDL event's source.ts_ms, handed to the parser through the new DDLParserService.setDdlEventTimestampMs (called in performDDLOperation); without a timestamp the back-fill cannot be computed and the statement is refused; - ADD COLUMN ENUM(..) NOT NULL without a DEFAULT gets DEFAULT '<first member>'; - any other non-literal ADD COLUMN default raises DDLReplicationException naming the column, the default and the remedy (apply by hand or re-snapshot, then ignore.ddl.regex); - MODIFY/CHANGE back-fill nothing, so their non-literal defaults are still dropped at INFO. Tests flipped deliberately because they pinned the divergence: testAlterDatabaseAddColumnEnum (ENUM NOT NULL now DEFAULT 'M'); testAddColumnDefaultCurrentTimestampIsDropped split into testModifyColumnDefaultCurrentTimestampIsDropped (unchanged behaviour), testAddColumnCurrentTimestampBackfillsLiteral and testAddColumnExpressionDefaultIsLoud. New: testAddEnumNotNullDefaultsToFirstMember, testBitStringAndHexDefaultsAreTranslated. * fix(ddl): compare sorting-key DateTime types without their timezone argument KeyColumnTypeChange.normalise only rewrote the translator's placeholder `DateTime64(p,0)` to `DateTime64(p)`. A key column created by the record path or with clickhouse.datetime.timezone renders in system.columns as `DateTime64(6, 'UTC')`, so a no-op `MODIFY COLUMN ts DATETIME(6) NOT NULL` compared `DateTime64(6, 'UTC')` with `DateTime64(6, 0)`, was judged "not comparable" and raised DDLReplicationException -- the pipeline stopped on a restatement that changes nothing. The zone is display metadata: the instants, width and scale are the same. normalise now strips the zone (and the , 0 placeholder) from DateTime64 and the zone from DateTime/DateTime32, so such a MODIFY is suppressed as a restatement (Spec 06.05 s3.4). A different scale is still not comparable and stays loud. Test: testModifyDateTimeKeyWithTimezoneIsSuppressed (red before the fix with the "not comparable" exception). * fix(ddl): MODIFY/CHANGE ... NOT NULL keeps the column's existing nullability instead of widening it `MODIFY COLUMN c BIGINT NOT NULL` on a column that is already non-Nullable in ClickHouse was emitted as `MODIFY COLUMN c Nullable(Int64)`: the NOT NULL branch honoured the constraint only for ADD COLUMN and left every MODIFY at the Nullable default. That is not what the source declares, and it turns a restatement into a real type change ClickHouse has to apply. Now (Spec 06.05 s3.2 rule 2) a NOT NULL MODIFY/CHANGE reads the target column's nullability: non-Nullable stays non-Nullable, Nullable stays Nullable (the Code: 36 avoidance is unchanged), unknown schema stays Nullable. Explicit NULL still widens. Test: testModifyNotNullKeepsNonNullableColumn (red before the fix: Nullable(Int64)); testAlterModifyColumnNotNullStaysNullable still pins the unknown-schema case. * fix(ddl): stop loudly when ADD/DROP PRIMARY KEY changes the row identity the replica is keyed by ADD PRIMARY KEY and DROP PRIMARY KEY were in the skip class unconditionally, and Spec 06.07 accepted the consequence: the ClickHouse table stayed keyed by the old columns while the source identified rows differently, so rows the source keeps distinct collapse under FINAL -- a count-clean divergence that surfaced only in db_compare. The production shape `DROP PRIMARY KEY, MODIFY old_id, ADD COLUMN new_id ... FIRST, ADD PRIMARY KEY (new_id)` translated to just the ADD COLUMN. Now (Spec 06.07 s3.1) the statement's net identity -- the last ADD PRIMARY KEY / column-level PRIMARY KEY, or a DROP without replacement -- is judged against the target sorting key (TargetSchemaLookup.sortingKeyTypes, the connector's own columns removed) once the clause list has been walked: - key unknown: the clauses stay skipped (unchanged); - restatement (same column set, case/quoting/order ignored): skipped; - otherwise DDLReplicationException naming both keys and the rebuild (re-create with the new ORDER BY, re-snapshot); nothing is emitted. A column-level PRIMARY KEY on ADD COLUMN also implies NOT NULL. Lean: Clause.primaryKeyChange joins the loud set; primary_key_change_is_loud proves any statement carrying it fails. Spec 06.03 s3.2 lists the clause in the Loud class; CONSTITUTION s5 and the Lean README updated. Test flipped deliberately because it pinned the quiet outcome: the keyed half of testAlterDropPrimaryKeyModifyKeyAddColumnAddPrimaryKey (now loud). New: testAddPrimaryKeyThatChangesIdentityIsLoud, testDropPrimaryKeyIsLoud (both red before the fix). The unknown-key tests are unchanged. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Defects fixed (spec first, then code; each with a red-then-green regression test and a mutation check)
Verification