[OPIK-7772] [BE] test: add the topology-aware traces DDL pattern and its reference migration - #7952
Merged
Merged
Conversation
github-actions
Bot
added
java
Contributor
⏱️ pre-commit per-hook timing
⏭️ 39 skipped (no matching files changed)
|
Merged
2 tasks
Collaborator
|
No test needed here. All 10 files are under apps/opik-backend/src/test — the schema-parity harness, the reference topology-aware migration and the unguarded counter-example are test fixtures, and no shipped changelog references traces-ddl-reference or traces-ddl-unguarded, so a running Opik never applies them. There is no runtime behaviour for an e2e test to observe, and the pattern this establishes is already asserted by the pre/post-cutover parity tests it ships with (including their negative cases). Nothing needed from QA here. Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 25 Aug 09:55 UTC — nothing the verdict depends on changed. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
thiagohora
force-pushed
the
thiagoh/OPIK-7772-ddl-pattern-reference
branch
from
August 21, 2026 16:02
f664c8a to
14239c5
Compare
thiagohora
force-pushed
the
thiagoh/OPIK-7772-ddl-pattern-reference
branch
from
August 21, 2026 16:44
d9a13e7 to
039831f
Compare
thiagohora
force-pushed
the
thiagoh/OPIK-7772-ddl-pattern-reference
branch
from
August 21, 2026 17:20
6904e7b to
e27cf4a
Compare
Base automatically changed from
thiagoh/OPIK-7772-traces-ddl-topology-guard
to
main
August 24, 2026 16:11
…d builders Review feedback on the type-parity round. **`is_deleted` was checked by nothing.** The type comparison iterates the columns of `traces` and looks each up on the shadow, so a shadow-only column is structurally unreachable by it — and it is not in the type allowlist either. That left the one column where the *default* is the entire contract completely unpinned: the cutover backfill deliberately omits `is_deleted` so it takes its default, so `DEFAULT 1` would materialise every copied row as a ReplacingMergeTree tombstone. Silent, total data loss at swap time. SHADOW_ONLY_COLUMNS is now a pinned contract (type, default kind, default expression, and the reason) rather than a bare name set, asserted in full, with a negative test flipping the default to 1. The name-set comparisons read its key set, so there is still one source of truth for what may differ. **Records use builders.** SKILL.md requires `@Builder(toBuilder = true)` and builder construction, and the rationale applies directly here rather than being ceremony: BaselineTypeDifference takes three same-typed Strings, so a positional swap of tracesType and shadowType would silently invert the assertion it exists to make. TableSchema and its nested Column / SkipIndex / Projection records get the same treatment — TableSchema's constructor took five consecutive Strings. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…its reference migration Establishes the shape every future `traces` schema change ships in: two complementary changesets guarded on the same runtime fact — whether `traces_local` exists — so exactly one branch executes and the other is recorded MARK_RAN. The guard is a formatted-SQL sqlCheck against system.tables rather than a tableExists precondition because it has to read the *runtime* topology, which no changelog records, and both branches are IF [NOT] EXISTS so any re-run or partially-applied branch is idempotent. The reference migration demonstrates the two cases we have historically shipped, one of each kind: a read-facing field (a MATERIALIZED column, applied to the shard and the Distributed wrapper) and a storage-only skip index (applied to the shard alone). A derived column is used deliberately — read-facing enough to exercise the wrapper branch, while needing no cutover-backfill entry, so the fixture never has to edit shipped cutover SQL. Both topology gates now apply the same single file and assert the correct branch executed while the other was recorded MARK_RAN — which also confirms liquibase-clickhouse 0.7.2 honours these preconditions, the fact the pattern rests on — that re-applying is a no-op, and that the field is genuinely readable through the wrapper afterwards. A negative-control fixture carries the same intent written the ordinary un-guarded way, as one unconditional ALTER TABLE traces. It applies cleanly on both topologies and is wrong on both — pre-cutover it never reaches the shadow, post-cutover it only reaches the wrapper — and both gates assert they reject it. That is what makes the pattern load-bearing rather than ceremony. The fixtures are kept off anything an install runs: they live under src/test/resources so the shipped changelog's includeAll cannot reach them, they carry no migration number, and their changeset author is `opik-7772-test-fixture` so the ledger rows they write in a throwaway container cannot collide with a shipped changeset id. Each gate additionally asserts the shipped changelog is still fully applied once a fixture has run, which revalidates its recorded checksums too. No shipped migration is added or edited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the ledger query
Review feedback on the DDL pattern fixtures.
The reference migration's ALTERs omitted `ON CLUSTER '{cluster}'`. That is a real
defect in a file whose entire purpose is to be copied: without it the DDL reaches
only the node the migration connected to, leaving other replicas short of the
column while the changeset is recorded as applied. It matters twice over here,
because the guard branch is chosen from a *local* system.tables read — so on a
divergent cluster one node can record MARK_RAN for a topology the others are not
in. Every shipped traces/spans ALTER already uses it, and migrations.md requires
it. Added to both branches and every rollback.
The un-guarded negative control gets it too, so that fixture is now written
correctly in every respect *except* the missing precondition guard. That keeps it
sharp: when the gates reject it, they are rejecting the absent guard and nothing
else.
TracesDdlReferenceFixture#execType now binds the changeset id and author through
a PreparedStatement instead of interpolating them. They are values in a predicate,
which is what SKILL.md's SQL rule reserves for binding. Identifiers elsewhere in
these gates stay interpolated because ClickHouse accepts no parameter in a table
or column position — that limitation is now stated where it applies rather than
left as an apparent inconsistency.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… contract Review feedback on the fixture assertions, which checked presence where they should have checked the contract. * The reference field is asserted on type and default kind (UInt64 MATERIALIZED) on every target in both topologies, not just by name. A column of that name arriving as UInt32, or as an ALIAS, would have satisfied a presence check while breaking what the migration declares. * The reference index is compared as a full SkipIndex record — set(0) on `name` at granularity 1 — so an index of the right name with the wrong type, expression or granularity now fails. The wrapper-absence check is unchanged. This matches what assertPreCutoverParity already does for shared indices and projections. * The post-cutover readability check evaluated nothing: SELECT ... LIMIT 0 proves the wrapper can resolve the column, not that the expression behind it works, so a materialized column with a valid name and a broken definition passed. One row is now written through the wrapper and its computed value read back — the reference declares MATERIALIZED length(name), so a known name must yield its length. The LIMIT 0 sweep over all 32 shard columns stays; that one is about the wrapper exposing the whole list, which is a different property. The expectations live on TracesDdlReferenceFixture beside the names they qualify, so both gates assert the same contract. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ust its kind Review feedback. The fixture contract pinned type and default kind but not the expression, so a MATERIALIZED UInt64 computing something other than length(name) satisfied every assertion while producing different values per topology — the same drift assertPostCutoverParity compares expressions to catch. Now asserted in both topology helpers against a single declared constant. Also standardised "un-guarded" to "unguarded" across the tests, fixtures and display names. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
thiagohora
force-pushed
the
thiagoh/OPIK-7772-ddl-pattern-reference
branch
from
August 25, 2026 08:36
89832f9 to
9cbbe8f
Compare
Contributor
Python SDK E2E Tests Results (Python 3.14)296 tests ±0 288 ✅ - 1 4m 50s ⏱️ +50s Results for commit 657dbcc. ± Comparison against base commit 634f315. This pull request skips 1 test.♻️ This comment has been updated with latest results. |
…ding titles Review feedback on the parity gates. **Cleanup is now guaranteed.** The negative tests restored the schema only after their assertions, so a failing one left its drift in place and every later @ordered test in the same container failed for a reason unrelated to what it asserts — burying the real failure. assertDriftIsCaught injects, asserts and restores in a finally, then re-asserts parity so the cleanup is proven rather than assumed. It also removes the repetition across seven negative tests; the projection case benefits most, since it restores two table settings as well as dropping the projections. **Teardown no longer masks setup failures.** If migration or connection setup threw before `connection` was assigned, @afterall's close() raised an NPE over the real error. Now null-safe, with the container stops in a finally so they run even if closing fails. **Two titles were wrong, and the second materially so.** "marks the other one run" understates MARK_RAN, which records a changeset as applied *without executing its statements*; the titles now say so. And "an unguarded traces migration is rejected" was backwards: the migration applies perfectly cleanly — that is the entire premise of this ticket — and it is the gate that rejects the drift it leaves behind. Retitled to say that, since a reader scanning test names would otherwise take away the opposite of the thing being demonstrated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
andrescrz
previously approved these changes
Aug 25, 2026
andrescrz
approved these changes
Aug 25, 2026
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
3 participants
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.
Details
Stack 2/4 for OPIK-7772 — base #7951, review that one first. Establishes the shape every future
tracesschema change ships in: two complementary changesets guarded on the same runtime fact — whethertraces_localexists — so exactly one branch executes and the other is recordedMARK_RAN. Both gates from #7951 now apply the same single file to their own topology and prove the correct branch ran, which is what turns the pattern from a convention into something CI can enforce.sqlCheckagainstsystem.tablesrather thantableExists, because the guard has to read the runtime topology, which no changelog records;onFail:MARK_RAN, so the skipped branch is marked applied without executing and a later startup never retries it against the wrong topology;onError:HALT, so an unevaluable precondition stops rather than guesses; andIF [NOT] EXISTSthroughout, so a re-run or partially-applied branch is idempotent.liquibase-clickhouse0.7.2 honours these preconditions — the fact the whole pattern rests on. A version bump that broke it now fails CI rather than production.MATERIALIZEDcolumn → shard and wrapper) and a storage-only skip index (→ shard alone). A derived column is used deliberately: read-facing enough to exercise the wrapper branch, while needing no cutover-backfill entry, so the fixture never has to edit shipped cutover SQL. The preserved-column → backfill-list obligation is already covered by a negative test in [OPIK-7772] [BE] test: guard traces DDL across pre/post-cutover topologies in CI #7951.ALTER TABLE traces. It applies cleanly on both topologies and is wrong on both — pre-cutover it never reaches the shadow, post-cutover it only reaches the wrapper — and both gates assert they reject it. That is what makes the pattern load-bearing rather than ceremony.src/test/resources, where the shipped changelog'sincludeAlloverliquibase/db-app-analytics/migrations/cannot reach them, so no deployment can apply them. They carry no migration number and use the changeset authoropik-7772-test-fixture, so a ledger row from a fixture is unmistakable — and since Liquibase identity is id + author + filename, and the fixture path differs from the shipped one, a collision was already impossible.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
TracesDdlReferenceFixtureconstants/ledger helper, and the fixture tests added to both gates. TheMARK_RANbehaviour and the fact that aMATERIALIZEDcolumn can be added to aDistributedwrapper were both verified empirically against a real container rather than assumed.Testing
Environment: local, Docker 29.7.2 (linux/aarch64), Corretto 25.0.3, Maven 3.9.9, from
apps/opik-backend.Result: 21/21 (pre-cutover 10, post-cutover 11), ~30s per gate.
Scenarios validated:
EXECUTEDand the post-cutover oneMARK_RAN; post-cutover, the reverse. Read from theDATABASECHANGELOGEXECTYPE, filtered by the fixture author.tracesand the shadow; post-cutover the field reaches shard and wrapper while the index reaches the shard only and is asserted absent from the wrapper.SELECTed through the wrapper, so the branch is proven to have produced a working column, not just matching metadata.Not run: the full backend suite (unchanged by this PR; CI runs it). No video — non-visual, test-only change.
Documentation
None in this PR. The playbook this reference implements lands in stack 4/4 as
apps/opik-backend/docs/traces-schema-ddl.md; the fixture headers already point at it.