[OPIK-7772] [BE] test: guard traces DDL across pre/post-cutover topologies in CI - #7951
Merged
Merged
Conversation
…ogies in CI The cutover to the partitioned, sharding-ready trace table is produced by the operator runbook, not by Liquibase, so the changelog and the runtime topology diverge the moment an install cuts over — and stay diverged for as long as the fleet is mixed. A `traces` schema change must therefore be correct against two physical layouts, and both failure modes are silent: post-cutover a shard-only ADD COLUMN applies without error but is unreadable through the Distributed wrapper, and a migration that alters `traces` but forgets the shadow leaves the next cutover copying a table that no longer matches. Adds the CI guard that turns both into merge-blocking failures: * TracesSchemaParityPreCutoverTest applies the changelog the way a fresh install does and asserts three-way parity — `traces`, the `traces_local_v2` shadow, and the shipped cutover backfill's INSERT column list, which is read from the reference SQL rather than restated so it cannot drift. * TracesSchemaParityPostCutoverTest stops the changelog after the shadow-table migration (000114), splices in the runbook's EXCHANGE + Distributed wrap, and resumes, so every later migration runs against the live post-cutover layout. It asserts the changelog applies with nothing left unrun, and that the wrapper exposes exactly the shard's columns. * Six negative tests inject the drift a careless migration would produce — a column or skip index on one table alone, a preserved column missing from the backfill list, a shard-only and a wrapper-only column — so no parity leg can silently stop firing. The shard-only case also pins the unreadability itself, and its counterpart pins the remedy, giving the "read-facing changes go to both" rule an executable demonstration. Parity compares the aspects a schema change moves (column sets, insertable columns, skip indices, projections, sorting/primary keys) and enumerates the shadow's deliberate extras; the baseline type/codec/partition differences stay owned by TracesLocalV2TableTest, TracesLocalV2BenchmarkTest and TracesLocalV2PartitioningTest. No shipped migration is edited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
Contributor
⏱️ pre-commit per-hook timing
⏭️ 40 skipped (no matching files changed)
|
Collaborator
|
No test needed here. This is a CI guard, not a product change — all 960 lines land in apps/opik-backend/src/test/java (TracesSchemaParity plus the pre/post-cutover suites and the MigrationUtils helper), and no changelog, DDL or runtime class moves. The traces/traces_local_v2 parity invariant it asserts is exactly the kind of drift an e2e test could never see anyway, so there is nothing here for another test to add. Worth saying: the negative tests that inject the drift and assert the guard rejects it are the part that makes this hold up. 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 21 Aug 17:21 UTC — nothing the verdict depends on changed. |
Merged
2 tasks
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
…dress review Review feedback on the topology guard. Parity here means column names and types and the select/expression definitions built on them — not the data, and not data lifecycle. Strengthened: * Projections are compared by full definition rather than by name. Two projections sharing a name but not a query would leave the successor keeping the name and losing the meaning, which a name-only check cannot see. * Post-cutover, columns are compared on their DEFAULT/MATERIALIZED expression as well as type and default kind. The wrapper is created AS the shard, so it starts an exact copy and any divergence is drift — a materialized column added to each side with a different expression previously satisfied every name and type assertion while computing something different on each. * The backfill's INSERT column list is now an ordered list with duplicates rejected, and is checked against its SELECT projection position by position. ClickHouse pairs the two by position and not by name, so a column added to one list and not the other sends every later value to the wrong destination column — no syntax error, and both tables stay perfectly consistent with each other, so no table-to-table comparison can see it. Each projection entry must name its destination (bare column, or an `AS <column>` alias), which the shipped SQL already does. Three new negative tests, and each new assertion was verified to fail before being committed green: * same-named projections with different queries. Worth noting: both trace tables are ReplacingMergeTree, which refuses ADD PROJECTION outright while deduplicate_merge_projection_mode is at its default (code 344) — so a real projection needs a deliberate per-table setting change. The test relaxes and restores it to make the leg reachable. * a materialized column whose expression differs between shard and wrapper. * the positional select check was mutation-tested by transposing two adjacent entries of the shipped SELECT projection, which it caught and every set-based comparison passed. Test hygiene, also from review: * The two one-sided column-drift tests are one @ParameterizedTest over (table, column); the flow was identical and only the target differed. * The shard-only-column pair no longer hands mutated schema between two @ordered tests. One test now owns the column from ADD to DROP, since the "after" half only means anything on the state the "before" half leaves behind. Deliberately not added: table TTL and storage policy. They are neither names, types nor selects, the changelog sets neither on the trace tables, and the tiered-storage policy is attached by an environment-gated migration outside this changelog — so a guard over the changelog could not meaningfully assert it. The scope boundary is now documented on TracesSchemaParity rather than left implicit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… wrap target
Second round of review on the topology guard. Both accepted findings were right,
and the first corrects an overreach in my earlier reasoning.
**Column types are now compared pre-cutover.** I had excluded per-column types
wholesale on the grounds that the shadow deliberately differs — but that
conflated types with codecs and defaults. Measured against the real schemas, only
6 of 31 shared columns differ in type (start_time, created_at, end_time, ttft,
duration, id_at), so the exemption is a short, enumerable list rather than most
of the table. BASELINE_TYPE_DIFFERENCES names those six with the reason each one
differs, and every other shared column is now type-checked. That catches a
precision narrowed on one table only, or a String quietly becoming
LowCardinality(String) on one side — both of which the name-set comparisons pass.
The allowlist is held honest in both directions: an entry whose columns no longer
differ fails as a stale entry, so it cannot decay into a blanket exemption for a
column nothing checks.
**The Distributed wrap target is pinned.** isDistributed() only checked the engine
prefix, so a wrapper over a different cluster, database, shard table or sharding
key would expose the same column list and pass everything. The engine is now
asserted to front `traces_local` in the same database on '{cluster}' with
sipHash64(project_id) — which also keeps the spliced statements honest against the
shipped 000003_exchange_and_wrap.sql they mirror.
Two negative tests, both verified to fail before being committed green: a
one-sided MODIFY COLUMN to LowCardinality(String), and a stale allowlist entry
produced by making the shadow's ttft Nullable again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Python SDK E2E Tests Results (Python 3.14)296 tests 287 ✅ 4m 58s ⏱️ For more details on these failures, see this check. Results for commit 7996d24. |
…ence Review feedback. The allowlist excused six columns from type parity and then only checked that their types still *differed*, so either side could drift to an unrelated type — `traces.start_time` becoming `String` — while still "differing" and so still being excused. Each entry is now a BaselineTypeDifference pinning the expected type on both `traces` and the shadow alongside the reason, and both are asserted. That closes the drift and makes the allowlist self-documenting: the entry states exactly what the difference is, not just that one exists. A converged pair still fails, so a dead exemption cannot linger over a column nothing checks. Two negative tests, one per side: the shadow's `ttft` made Nullable again (which also removes the difference), and `traces.start_time` narrowed to DateTime64(3) — the case a "they must differ" check would pass. Also two Javadoc grammar fixes from the same review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment on lines
+117
to
+119
| static final Map<String, BaselineTypeDifference> BASELINE_TYPE_DIFFERENCES = Map.of( | ||
| "start_time", new BaselineTypeDifference("DateTime64(9, 'UTC')", "DateTime64(6, 'UTC')", | ||
| "nanosecond -> microsecond precision; nothing ingested needs finer (000101)"), |
Contributor
There was a problem hiding this comment.
New test record bypasses builder convention
The BASELINE_TYPE_DIFFERENCES allowlist constructs BaselineTypeDifference with new instead of its required builder — should we add @Builder(toBuilder = true) to BaselineTypeDifference and use BaselineTypeDifference.builder() at these call sites, as .agents/skills/opik-backend/SKILL.md requires?
Want Baz to fix this for you? Activate Fixer You can also update your AI coding guidelines based on this comment by apply pr to [branch name]
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/db/TracesSchemaParity.java` around lines
117-119, update the `BASELINE_TYPE_DIFFERENCES` allowlist and the
`BaselineTypeDifference` record definition around lines 136 to follow the backend DTO
construction contract. Add `@Builder(toBuilder = true)` to the record and replace every
direct `new BaselineTypeDifference(...)` call in the allowlist with
`BaselineTypeDifference.builder()` construction, preserving all field values and adding
any required import.
| // Each allowlisted difference is pinned on both sides. Merely requiring the types to differ would excuse an | ||
| // unrelated drift on either table, and an entry whose columns have converged is a dead exemption that must be | ||
| // removed rather than left covering a column nothing checks. | ||
| BASELINE_TYPE_DIFFERENCES.forEach((name, expected) -> { |
Contributor
There was a problem hiding this comment.
Pre-cutover guard permits deleted backfill rows
The new pinning pass skips is_deleted because it iterates only BASELINE_TYPE_DIFFERENCES, so is_deleted UInt8 DEFAULT 1 can pass parity and materialize every copied row as deleted — should we pin its UInt8, DEFAULT, and 0 in the pre-cutover check, including the existing check around lines 228-233?
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/db/TracesSchemaParity.java` around lines
196 and 228-233, update the pre-cutover parity logic so the shadow-only `is_deleted`
column is not validated only by name. Pin and assert its exact shadow DDL semantics:
type `UInt8`, a `DEFAULT` clause, and default expression `0`, preventing a definition
such as `UInt8 DEFAULT 1` from passing while marking all copied rows deleted. Keep the
generic column-set validation, but add the explicit metadata checks alongside the
existing shadow-only handling.
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 1/4 for OPIK-7772 — base
main. The traces cutover is produced by the operator runbook, not by Liquibase, so from the moment an install cuts over the changelog and the runtime topology disagree, and the fleet stays mixed for months. Atracesschema change must therefore be correct against two physical layouts, and both ways of getting it wrong are silent — they raise nothing at migration time. This adds the CI guard that turns them into merge-blocking failures.ADD COLUMNapplies without error but is unreadable through theDistributedwrapper (code 47); and a migration that alterstracesbut forgets the shadow also passes, because the shadow is empty and nothing reads it until the cutover copies into it.TracesSchemaParityPreCutoverTestapplies the real changelog as a fresh install does, then asserts three-way parity:traces≅ thetraces_local_v2shadow ≅ the shipped cutover backfill'sINSERTcolumn list.TracesSchemaParityPostCutoverTeststops the changelog after the shadow-table migration (000114), splices in the runbook'sEXCHANGE+Distributedwrap, then resumes — so every later migration, including whichever one a PR is adding, runs on the live post-cutover topology.traces's insertable columns, and the shadow's are those plusis_deleted, so all three legs are exact assertions rather than fuzzy allowances.TracesLocalV2TableTest,TracesLocalV2BenchmarkTestandTracesLocalV2PartitioningTest— comparing them here would fail on every run.tracesand the negative tests drift it further.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
TableSchemasystem-table snapshot helper, the sharedTracesSchemaParityinvariant, and the partial-apply / unrun-changeset helpers added toMigrationUtils. The parity model was derived from the real schemas by dumpingsystem.columns/system.data_skipping_indices/system.tablesfrom a migrated container first, not assumed. The splicedEXCHANGE+ wrap statements mirror the shipped000003_exchange_and_wrap.sqland the already-validated inline statements inTracesLocalV2CutoverTest.Testing
Environment: local, Docker 29.7.2 (linux/aarch64), Corretto 25.0.3, Maven 3.9.9, from
apps/opik-backend.Results: pre-cutover 6/6, post-cutover 7/7 at the time of this commit,
ChangelogRebaselineTest2/2 unchanged. Roughly 30s per gate.Scenarios validated:
000114tolerates the wrapped layout.SELECTof every shard column through the wrapper to prove the measured parity is the parity that matters.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 these gates enforce is documented in stack 4/4, which also adds a pointer from
apps/opik-backend/AGENTS.md.