[OPIK-7772] [BE] refactor: make the resolver the only way to name a trace mutation's table - #7953

…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>

…race mutation's table

Post-cutover `traces` is a Distributed table, which rejects mutations (code 36 /
48), so every trace mutation must target the `traces_local` shard once the wrap
is live and `traces` while it is not. Routing was a two-branch
`<if(distributed_wrap)>traces_local<else>traces<endif>` conditional repeated in
every mutation template, plus one site that hand-rolled the same ternary in a
StringBuilder — which made a correct new mutation a matter of remembering to copy
the branch, and an incorrect one indistinguishable from a correct one at a glance.

Funnels the decision into TraceDAOImpl#tracesMutationTable(), the single place the
name is chosen:

* the mutation templates become topology-agnostic (`DELETE FROM
  <traces_mutation_table>`), with the resolver binding the resolved name;
* deleteForRetentionBounded appends the resolver's result instead of branching on
  the flag itself — it was the one site outside the resolver reading it.

Two guards keep it that way. TraceMutationRoutingArchTest asserts the flag is
read in exactly one place and the routing decision made in exactly one place;
these rules select the guarded method rather than its callers, so unlike
TraceDeletionEventArchTest they deliberately omit allowEmptyShould — an empty
selection would mean the method was renamed and the rule had stopped guarding.
TraceMutationSqlRoutingTest covers what a call-graph rule cannot see, a mutation
that hardcodes a table without consulting the flag at all: it reads the SQL
constants reflectively and also scans the source's single-line string literals,
which is the form the previous StringBuilder site took.

All four rules were verified to fail on injected violations before being
committed green, and TracesDistributedWrapMutationTest — which drives the delete
and retention paths against a real Distributed wrapper — passes unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…e unwrapped branch

Review feedback on the mutation-routing PR. All three findings were real.

**The bounded retention delete is now a declared template.** It was still built
with a StringBuilder, which SKILL.md forbids outright and which hid the statement
from both its declaration site and the routing guard that reads these constants.
DELETE_FOR_RETENTION_BOUNDED now sits beside its unbounded sibling, with the
OR-ed per-workspace predicates as a `getQueryItemPlaceHolder` template loop —
the same idiom BATCH_INSERT and the other variable-arity queries in this DAO use.
Every value stays bound; the table still comes from the resolver placeholder.

**The unwrapped branch of the resolver had no coverage at all.** The reviewer was
right, and more sharply than it first appeared: `TracesDistributedWrapMutationTest`
only runs with the wrap on, and nothing else in the repository calls
`deleteForRetentionBounded` — so the SQL this PR rewrites had no test on the
default topology. `TracesUnwrappedMutationTest` is its pre-cutover counterpart:
delete-by-id, both retention sweeps, a multi-workspace case that forces the
template loop to render more than one branch and its separator, and a guard
asserting `traces` is a MergeTree and `traces_local` does not exist, so a
mis-routed mutation could not have quietly succeeded.

Verified by inverting the resolver's ternary: 4 of 5 tests fail with "Table
opik.traces_local does not exist", which is precisely the bug that used to stay
green.

**The SQL detector missed qualified and quoted targets.** `analytics.traces`
reduced to `analytics` and was not flagged, so a qualified mutation escaped the
guard entirely. `normalizeTarget` now strips quoting, drops the database
qualifier and trims trailing punctuation, with 16 parameterized cases covering
qualified, backtick-quoted, double-quoted, upper-case and semicolon-terminated
forms, plus the complement that must not be flagged — the resolver placeholder,
`traces_local_v2`, `traces_pre_cutover_backup`, `trace_threads` and `spans`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…ention delete

Review feedback: the multi-workspace test seeded traces in only one workspace, so
it rendered the extra OR branches without ever verifying they select correctly. A
broken separator, a mis-numbered bind, or a single shared floor would all have
passed it.

It now seeds two workspaces through the public API and asserts *selective*
deletion: both traces sit inside the shared week window, so the toMonday bounds
cannot be what separates them — the only thing that can is each workspace's own
`:lb_i`. The first workspace's floor sits below its trace, the second's above its
own, and one call must delete the first and spare the second.

Verified load-bearing by collapsing the per-workspace bind to the first
workspace's floor: the test fails on "its workspace's floor sits above this trace,
so the same statement must spare it".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… its engine

Review feedback, both fair.

The suite asserts the pre-cutover topology as a *precondition*, so it has to own
that topology rather than inherit whatever a shared container is in. Reuse is
enabled in CI, so a container left wrapped by anything would have failed this
suite for environmental reasons rather than real ones. Now dedicated, non-reused
ClickHouse and ZooKeeper on their own network, matching
TracesDistributedWrapMutationTest, and stopped in afterAll.

The topology guard also accepted too much: `doesNotContain("Distributed")` passes
for the helper's "" sentinel when the table is missing, and for any other engine.
Pinned to ReplicatedReplacingMergeTree, so an absent or wrong table fails.

Renamed deleteForRetentionBoundedAppliesPerWorkspaceLowerBounds — the previous
name mangled the possessive.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…asserts

Review feedback. When I tightened the assertion from "not Distributed" to
ReplicatedReplacingMergeTree, the test name and its Javadoc kept saying "a plain
MergeTree" — which is both weaker and, for a replicated table, wrong. The name,
display name and surrounding docs now say ReplicatedReplacingMergeTree, matching
what the test enforces.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>