Skip to content

Commit d9a9621

Browse files
thiagohoraclaude
andauthored
[OPIK-7772] [BE] test: guard traces DDL across pre/post-cutover topologies in CI (#7951)
* [OPIK-7772] [BE] test: guard traces DDL across pre/post-cutover topologies 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> * [OPIK-7772] [BE] test: extend traces parity to select definitions; address 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> * [OPIK-7772] [BE] test: add column-type parity and pin the Distributed 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> * [OPIK-7772] [BE] test: pin both sides of each allowlisted type difference 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> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 684a1ce commit d9a9621

5 files changed

Lines changed: 1349 additions & 0 deletions

File tree

apps/opik-backend/src/test/java/com/comet/opik/api/resources/utils/MigrationUtils.java

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package com.comet.opik.api.resources.utils;
22

3+
import liquibase.Contexts;
4+
import liquibase.LabelExpression;
35
import liquibase.Liquibase;
6+
import liquibase.changelog.ChangeSet;
47
import liquibase.database.DatabaseConnection;
58
import liquibase.database.DatabaseFactory;
69
import liquibase.database.jvm.JdbcConnection;
@@ -14,6 +17,7 @@
1417
import ru.yandex.clickhouse.ClickHouseConnectionImpl;
1518

1619
import java.sql.SQLException;
20+
import java.util.List;
1721
import java.util.Map;
1822

1923
@UtilityClass
@@ -48,6 +52,82 @@ public static void runClickhouseDbMigration(ClickHouseContainer container) {
4852
}
4953
}
5054

55+
/**
56+
* Applies the ClickHouse changelog only up to and including the changesets of {@code migrationFileName}, leaving
57+
* every later migration unrun so a caller can transform the schema mid-changelog and then resume with
58+
* {@link #runClickhouseDbMigration(ClickHouseContainer)}.
59+
* <p>
60+
* This exists for the post-cutover topology gate: the cutover's {@code EXCHANGE} + {@code Distributed} wrap is
61+
* produced by the operator runbook rather than by Liquibase, so the only way to run the later migrations against
62+
* the topology they will really meet in production is to stop the changelog at the shadow-table migration, splice
63+
* the transform in, and carry on. The cut is expressed as a migration <i>file name</i> rather than a changeset
64+
* count so appending migrations never silently moves it.
65+
*
66+
* @param migrationFileName the migration file the apply stops after, e.g.
67+
* {@code 000114_recreate_traces_local_v2_id_at_datetime64.sql}
68+
*/
69+
public static void runClickhouseDbMigrationThrough(ClickHouseContainer container, String migrationFileName) {
70+
try (var connection = container.createConnection("")) {
71+
DatabaseConnection dbConnection = new JdbcConnection(
72+
new ClickHouseConnectionImpl(connection.getMetaData().getURL()));
73+
var database = DatabaseFactory.getInstance().findCorrectDatabaseImplementation(dbConnection);
74+
try (var liquibase = new Liquibase(CLICKHOUSE_CHANGELOG_FILE, new ClassLoaderResourceAccessor(),
75+
database)) {
76+
ClickHouseContainerUtils.migrationParameters().forEach(liquibase::setChangeLogParameter);
77+
liquibase.update(countChangeSetsThrough(liquibase, migrationFileName), new Contexts(),
78+
new LabelExpression());
79+
}
80+
} catch (SQLException e) {
81+
throw new RuntimeException("Failed to run ClickHouse DB migration", e);
82+
} catch (LiquibaseException e) {
83+
throw new UnexpectedLiquibaseException(e);
84+
}
85+
}
86+
87+
/**
88+
* Identifiers of the ClickHouse changesets the database has <b>not</b> applied. Empty means the changelog is fully
89+
* applied — which is the assertion a topology gate needs after resuming a spliced apply, because a changeset the
90+
* extension skipped (a precondition evaluating to {@code MARK_RAN} is recorded as run; an unsupported statement is
91+
* not) would otherwise leave the schema short without anything throwing.
92+
*/
93+
public static List<String> unrunClickhouseChangeSetIds(ClickHouseContainer container) {
94+
try (var connection = container.createConnection("")) {
95+
DatabaseConnection dbConnection = new JdbcConnection(
96+
new ClickHouseConnectionImpl(connection.getMetaData().getURL()));
97+
var database = DatabaseFactory.getInstance().findCorrectDatabaseImplementation(dbConnection);
98+
try (var liquibase = new Liquibase(CLICKHOUSE_CHANGELOG_FILE, new ClassLoaderResourceAccessor(),
99+
database)) {
100+
ClickHouseContainerUtils.migrationParameters().forEach(liquibase::setChangeLogParameter);
101+
return liquibase.listUnrunChangeSets(new Contexts(), new LabelExpression())
102+
.stream()
103+
.map(ChangeSet::getId)
104+
.toList();
105+
}
106+
} catch (SQLException e) {
107+
throw new RuntimeException("Failed to list unrun ClickHouse changesets", e);
108+
} catch (LiquibaseException e) {
109+
throw new UnexpectedLiquibaseException(e);
110+
}
111+
}
112+
113+
/**
114+
* Number of changesets from the start of the changelog through the last one declared in {@code migrationFileName}.
115+
* The changelog is a single {@code includeAll}, so this is the file's position in lexicographic order; counting the
116+
* parsed changesets rather than the files keeps it right for a migration that declares more than one.
117+
*/
118+
private static int countChangeSetsThrough(Liquibase liquibase, String migrationFileName)
119+
throws LiquibaseException {
120+
var changeSets = liquibase.getDatabaseChangeLog().getChangeSets();
121+
for (int i = changeSets.size() - 1; i >= 0; i--) {
122+
if (changeSets.get(i).getFilePath().endsWith(migrationFileName)) {
123+
return i + 1;
124+
}
125+
}
126+
throw new IllegalArgumentException(
127+
"No changeset found for migration file '%s' in %s".formatted(migrationFileName,
128+
CLICKHOUSE_CHANGELOG_FILE));
129+
}
130+
51131
private static void runDbMigration(String changeLogFile, Map<String, String> parameters,
52132
DatabaseConnection connection) {
53133
try {
Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
package com.comet.opik.db;
2+
3+
import java.sql.Connection;
4+
import java.sql.ResultSet;
5+
import java.sql.SQLException;
6+
import java.util.ArrayList;
7+
import java.util.LinkedHashMap;
8+
import java.util.LinkedHashSet;
9+
import java.util.List;
10+
import java.util.Map;
11+
import java.util.Set;
12+
import java.util.stream.Collectors;
13+
14+
/**
15+
* A {@code SHOW CREATE}-level snapshot of one ClickHouse table, read from the {@code system} tables so it reflects the
16+
* schema the server actually holds rather than the DDL someone believes it applied.
17+
*
18+
* <p>It carries every aspect a schema change can touch — columns (with their type, DEFAULT/MATERIALIZED kind and
19+
* expression, and compression codec), data-skipping indices, the sorting / primary / partition keys, projections, and
20+
* the engine — so a guard comparing two tables can assert on all of them instead of on column names alone. Parsing
21+
* {@code SHOW CREATE TABLE} text would carry the same information but compare formatting as well as substance; the
22+
* {@code system} tables give the same facts already decomposed.
23+
*
24+
* <p>{@code columns} keeps the server's declaration order ({@code system.columns.position}), which matters because two
25+
* tables can hold the same column set in a different order — the trace shard and its shadow do exactly that. Callers
26+
* comparing sets should go through {@link #columnNames()} / {@link #storedColumnNames()} rather than comparing the
27+
* lists.
28+
*/
29+
record TableSchema(
30+
String table,
31+
String engine,
32+
String partitionKey,
33+
String sortingKey,
34+
String primaryKey,
35+
List<Column> columns,
36+
List<SkipIndex> skipIndices,
37+
List<Projection> projections) {
38+
39+
/**
40+
* @param defaultKind {@code DEFAULT}, {@code MATERIALIZED}, {@code ALIAS}, or empty when the column simply has no
41+
* default. The distinction is load-bearing: only a non-{@code MATERIALIZED}/{@code ALIAS} column can be
42+
* named in an {@code INSERT}, so it is what separates a column the cutover backfill must carry from one
43+
* the destination recomputes for itself.
44+
*/
45+
record Column(String name, String type, String defaultKind, String defaultExpression, String codec) {
46+
}
47+
48+
record SkipIndex(String name, String typeFull, String expression, long granularity) {
49+
}
50+
51+
record Projection(String name, String query) {
52+
}
53+
54+
private static final Set<String> COMPUTED_DEFAULT_KINDS = Set.of("MATERIALIZED", "ALIAS");
55+
56+
static TableSchema read(Connection connection, String database, String table) throws SQLException {
57+
var tableRow = readTableRow(connection, database, table);
58+
return new TableSchema(
59+
table,
60+
tableRow.get("engine_full"),
61+
tableRow.get("partition_key"),
62+
tableRow.get("sorting_key"),
63+
tableRow.get("primary_key"),
64+
readColumns(connection, database, table),
65+
readSkipIndices(connection, database, table),
66+
readProjections(connection, database, table));
67+
}
68+
69+
/** Column names in the server's declaration order. */
70+
List<String> columnNames() {
71+
return columns.stream().map(Column::name).toList();
72+
}
73+
74+
/**
75+
* The columns an {@code INSERT} can name — everything except {@code MATERIALIZED} / {@code ALIAS}, which the server
76+
* computes and refuses to accept a value for.
77+
*/
78+
Set<String> storedColumnNames() {
79+
return columns.stream()
80+
.filter(column -> !COMPUTED_DEFAULT_KINDS.contains(column.defaultKind()))
81+
.map(Column::name)
82+
.collect(Collectors.toCollection(LinkedHashSet::new));
83+
}
84+
85+
Map<String, Column> columnsByName() {
86+
var byName = new LinkedHashMap<String, Column>();
87+
columns.forEach(column -> byName.put(column.name(), column));
88+
return byName;
89+
}
90+
91+
Map<String, SkipIndex> skipIndicesByName() {
92+
var byName = new LinkedHashMap<String, SkipIndex>();
93+
skipIndices.forEach(index -> byName.put(index.name(), index));
94+
return byName;
95+
}
96+
97+
Set<String> skipIndexNames() {
98+
return skipIndicesByName().keySet();
99+
}
100+
101+
Set<String> projectionNames() {
102+
var names = new LinkedHashSet<String>();
103+
projections.forEach(projection -> names.add(projection.name()));
104+
return names;
105+
}
106+
107+
boolean isDistributed() {
108+
return engine.startsWith("Distributed");
109+
}
110+
111+
private static Map<String, String> readTableRow(Connection connection, String database, String table)
112+
throws SQLException {
113+
var sql = """
114+
SELECT engine_full, partition_key, sorting_key, primary_key
115+
FROM system.tables WHERE database = '%s' AND name = '%s'
116+
""".formatted(database, table);
117+
try (var statement = connection.createStatement(); var resultSet = statement.executeQuery(sql)) {
118+
if (!resultSet.next()) {
119+
throw new IllegalStateException("Table '%s.%s' does not exist".formatted(database, table));
120+
}
121+
return Map.of(
122+
"engine_full", text(resultSet, "engine_full"),
123+
"partition_key", text(resultSet, "partition_key"),
124+
"sorting_key", text(resultSet, "sorting_key"),
125+
"primary_key", text(resultSet, "primary_key"));
126+
}
127+
}
128+
129+
private static List<Column> readColumns(Connection connection, String database, String table) throws SQLException {
130+
var sql = """
131+
SELECT name, type, default_kind, default_expression, compression_codec
132+
FROM system.columns WHERE database = '%s' AND table = '%s' ORDER BY position
133+
""".formatted(database, table);
134+
var columns = new ArrayList<Column>();
135+
try (var statement = connection.createStatement(); var resultSet = statement.executeQuery(sql)) {
136+
while (resultSet.next()) {
137+
columns.add(new Column(
138+
text(resultSet, "name"),
139+
text(resultSet, "type"),
140+
text(resultSet, "default_kind"),
141+
text(resultSet, "default_expression"),
142+
text(resultSet, "compression_codec")));
143+
}
144+
}
145+
return List.copyOf(columns);
146+
}
147+
148+
private static List<SkipIndex> readSkipIndices(Connection connection, String database, String table)
149+
throws SQLException {
150+
var sql = """
151+
SELECT name, type_full, expr, granularity
152+
FROM system.data_skipping_indices WHERE database = '%s' AND table = '%s' ORDER BY name
153+
""".formatted(database, table);
154+
var indices = new ArrayList<SkipIndex>();
155+
try (var statement = connection.createStatement(); var resultSet = statement.executeQuery(sql)) {
156+
while (resultSet.next()) {
157+
indices.add(new SkipIndex(
158+
text(resultSet, "name"),
159+
text(resultSet, "type_full"),
160+
text(resultSet, "expr"),
161+
resultSet.getLong("granularity")));
162+
}
163+
}
164+
return List.copyOf(indices);
165+
}
166+
167+
private static List<Projection> readProjections(Connection connection, String database, String table)
168+
throws SQLException {
169+
var sql = """
170+
SELECT name, query FROM system.projections
171+
WHERE database = '%s' AND table = '%s' ORDER BY name
172+
""".formatted(database, table);
173+
var projections = new ArrayList<Projection>();
174+
try (var statement = connection.createStatement(); var resultSet = statement.executeQuery(sql)) {
175+
while (resultSet.next()) {
176+
projections.add(new Projection(text(resultSet, "name"), text(resultSet, "query")));
177+
}
178+
}
179+
return List.copyOf(projections);
180+
}
181+
182+
/** ClickHouse returns an absent String as {@code ""}; normalise the JDBC {@code null} case to match. */
183+
private static String text(ResultSet resultSet, String column) throws SQLException {
184+
var value = resultSet.getString(column);
185+
return value == null ? "" : value;
186+
}
187+
}

0 commit comments

Comments
 (0)