[OPIK-8230] [BE] fix: scope the trace delete mutation to the batch's own partitions - #8131
[OPIK-8230] [BE] fix: scope the trace delete mutation to the batch's own partitions#8131thiagohora wants to merge 1 commit into
Conversation
… partitions
A ClickHouse mutation is registered against every active part of the target
table before its WHERE clause is even considered - the existing partition
predicate (OPIK_6901) prunes which rows a matched part rewrites, not which
parts the mutation visits at all. Post-cutover traces_local has ~1,900
partitions / ~3,650 parts, so an ordinary delete matching a handful of rows
was registered against all of them, at ~19ms/part fixed overhead. That is
what pushed deletes past max_execution_time (Code 159 TIMEOUT_EXCEEDED),
where the statement errors and skips the TracesDeleted cascade and the
deletion-events bridge write even though the mutation completes and the
rows are in fact deleted.
Scope each statement to the partitions its own batch resolves to via
IN PARTITION, derived by WeeklyPartitions#groupByPartition (new; groups
ids by partition rather than naming their union - IN PARTITION accepts
exactly one partition per statement). One statement per partition,
falling back to today's unbounded form whenever the batch contains an id
whose partition cannot be derived exactly, or whenever the mutation's
target table is not actually partitioned (the legacy pre-wrap traces has
no partition key at all, so IN PARTITION against it is a ClickHouse
syntax error - Code 248 INVALID_PARTITION_VALUE - not a no-op). The
routing decision stays in exactly one place: deleteBatch calls
tracesMutationTable() itself, resolved once and reused across every
partition-scoped statement in the batch, never the wrap flag directly
(TraceMutationRoutingArchTest enforces this).
Both forms share one SQL template, DELETE_BY_PROJECT_ID_TRACE_ID_PAIRS,
with an <if(partition)>IN PARTITION <partition><endif> conditional -
scoped and unbounded no longer duplicate their WHERE/SETTINGS body across
two near-identical constants, matching the pre-diff code's own pattern
for its now-removed <if(partitions)> fragment. executeDelete lost its
sqlTemplate parameter along with the second constant, since there is now
only one to pass.
WeeklyPartitions#of, the flat-union predecessor this class shipped with,
is removed - groupByPartition is its only production caller's need, and
of() had none left once TraceDAO switched over. Its unique boundary-case
coverage (ceiling/floor ids, largest UUIDv7 timestamp) was migrated to
groupByPartition rather than dropped; the rest was redundant with
groupByPartition's own tests using the same pinned ids. Every stale
{@link WeeklyPartitions#of} javadoc reference left over from the rename
now points at #groupByPartition.
TracesLegacyTablePruningMutationTest's nonV7IdDisablesPruning assertion
was still matching against the retired row-level predicate regex - a
tautology that could never fail regardless of correctness, since the new
SQL shape can't contain it either way. Migrated to the same
doesNotContain("IN PARTITION") check its two sibling assertions in the
same file already use, and the now-fully-dead PARTITION_PREDICATE /
EMITTED_IN_CLAUSE / HONEST_WEEK / LEGACY_WEEK / boundPartitionsOf this
left behind were removed with it.
Two known, accepted tradeoffs, documented at their call sites rather than
addressed here since both are out of this ticket's stated scope:
- TraceService's delete->cascade coupling (TracesDeleted publish +
deletion-events capture) is still all-or-nothing over a batch that can
now complete several partitions' statements before one errors, so more
of a batch's rows can be synchronously, confirmedly deleted without the
cascade firing than before. Deletes are idempotent and this is the
ticket's own explicit boundary ("removes the condition that trips it;
does not restructure the coupling").
- The IN PARTITION gate is a name check (tracesMutationTable() ==
traces_local), not a runtime read of the table's actual partition key -
an operational precondition the class already relies on for every other
mutation, made explicit rather than newly introduced.
Verified locally against real ClickHouse via testcontainers:
- WeeklyPartitionsTest: 17/17
- TracesPartitionPruningMutationTest: 8/8, rewritten to assert on
system.part_log MutatePart events rather than EXPLAIN (which reports
read-planner pruning, a different layer from what a mutation actually
visits - the gap the old coverage fell into)
- TracesLegacyTablePruningMutationTest: 3/3, updated for the corrected
pre-wrap behavior
- TraceMutationRoutingArchTest: 3/3 - the routing decision is still made
in exactly one place
Not yet exercised: the TracesDeleted cascade and deletion_events_local
write firing again once the statement stops erroring, and the e2e specs -
none reachable without the full cascade path or the e2e suite running.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
java
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
|
Worth a test, but not testable yet. The IN PARTITION scoping only runs when tracesMutationTable() resolves to traces_local — i.e. ANALYTICS_DB_DATA_MODEL_TRACES_DISTRIBUTED_WRAP_ENABLED=true over the post-cutover partitioned table. Our e2e estate is the OSS docker-compose default (flag false, MergeTree traces), so deleteBatch always takes the grouped.isEmpty() fallback there and no Playwright spec can reach the new path; TracesPartitionPruningMutationTest already covers it where it is reachable, hand-authoring the topology to do so. Recording this as deferred rather than 'no test needed' because post-cutover a mis-derived partition deletes nothing and still returns 2xx — a silent skip is exactly what an e2e delete spec should catch, and nothing will re-triage this when the wrap is flipped in a later PR. Separately, the OSS-reachable half is already guarded: trace-explore/trace-delete.spec.ts. Testable once (platform): An estate where traces is the Distributed wrapper over a weekly-partitioned traces_local AND ANALYTICS_DB_DATA_MODEL_TRACES_DISTRIBUTED_WRAP_ENABLED=true. Neither half alone is enough: docker-compose defaults the flag to false, and ClickHouseTracesTopologyHealthCheck fails readiness if the flag is flipped without the cutover EXCHANGE having run. What a test would assert: Bulk delete on the Logs page and DELETE /v1/private/traces/delete. Post-cutover a wrong partition derivation is silent, not loud: the statement names a partition the row is not in, matches nothing, and the request still succeeds — the trace reappears on reload. The axis a test must vary is the deployment topology (unpartitioned traces vs the Distributed wrap over weekly-partitioned traces_local), not the request or the caller; and the second axis is the id's embedded UUIDv7 week, since the partition is derived from it — a present-day id resolves to one partition, an id past the DateTime 32-bit modulus to two, and a non-UUIDv7 or post-ceiling id to the unbounded fallback. Recorded rather than dropped, so this resurfaces when the gate opens — a flag is usually flipped by a PR that touches no product code and so is never triaged on its own. Also already tested. The behaviour this PR keeps on the default OSS topology — the unbounded fallback, byte-identical to pre-OPIK-8230 — is covered by tests_end_to_end/e2e/tests/trace-explore/trace-delete.spec.ts, which bulk-deletes two of three traces from the Logs table and re-deletes via the REST API, asserting in both cases that the targeted traces are gone from the UI and return null from GET /traces/{id} while the survivor remains. That would fail loudly if the TRACES_LOCAL_TABLE gate in deleteBatch were wrong and IN PARTITION reached the unpartitioned traces (Code 248), which is the trap your own Javadoc calls out. It does not distinguish the scoped path from the fallback — on this estate only the fallback ever runs — so it is regression cover for the refactor, not for the fix. also touches Backend (Java API / internal) Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. |
Backend Tests - Integration Group 9 46 files - 1 46 suites - 1 11m 5s ⏱️ - 1m 13s For more details on these failures, see this check. Results for commit cf0ebe7. ± Comparison against base commit 867d65e. This pull request removes 18 and adds 10 tests. Note that renamed tests count towards both. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| delete(Set.of(Pair.of(projectId, ids.getFirst()))); | ||
| var bounded = partsSelectedBy(lastTraceDeleteSql(ids.getFirst())) | ||
| .orElseThrow(() -> new AssertionError( | ||
| "EXPLAIN reported no partition index for the bounded delete")); | ||
|
|
||
| // Unbounded: a non-v7 id in the batch, so no predicate at all. Its partner is a different era, so the query_log | ||
| // lookup finds this statement rather than the one above. | ||
| var partner = ids.get(1); | ||
| delete(Set.of(Pair.of(projectId, partner), Pair.of(projectId, NON_V7_ID))); | ||
| var unbounded = partsSelectedBy(lastTraceDeleteSql(partner)); | ||
|
|
||
| assertThat(bounded.selected()) | ||
| .as("the bounded delete selects fewer parts than the table holds: %s", bounded) | ||
| .isLessThan(bounded.total()); | ||
| // Shown to discriminate, or it proves nothing - the same trap as `.contains(partition)` and | ||
| // `doesNotContain("toDayOfWeek")`. The fallback must not prune: either the planner reports no partition index at | ||
| // all, because nothing filters on the key, or it reports every part still selected. | ||
| assertThat(unbounded.map(parts -> parts.selected() == parts.total()).orElse(true)) | ||
| .as("the fallback prunes nothing: %s", unbounded) | ||
| .isTrue(); | ||
| var bounded = mutatePartActivitySince(table, boundedSince); |
There was a problem hiding this comment.
Pruning test is timing-flaky
bounded.parts()/partitions() and the unbounded activity snapshot are read immediately after DELETE FROM, but mutatePartActivitySince only runs SYSTEM FLUSH LOGS, which doesn't wait for MutatePart events, so snapshots can be incomplete and footprint assertions flaky. Could we wait for the target table's mutations to settle after each delete before taking each snapshot, while retaining since before submission?
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/infrastructure/TracesPartitionPruningMutationTest.java`
around lines 835-836 and 848-849, update `pruningReachesThePlannerAndTheFallbackDoesNot`
so it does not snapshot `system.part_log` immediately after asynchronous deletes. Keep
each `since` timestamp before submitting its delete, then call
`waitForMutationsToSettle(table)` after each delete and before
`mutatePartActivitySince(...)`, ensuring the bounded and unbounded activity measurements
are complete and deterministic.
| return makeMonoContextAware((userName, workspaceId) -> Flux | ||
| .fromIterable(Lists.partition(List.copyOf(projectIdTraceIdPairs), ANALYTICS_DELETE_BATCH_SIZE)) | ||
| .concatMap(batch -> { | ||
| var template = getSTWithLogComment(DELETE_BY_PROJECT_ID_TRACE_ID_PAIRS, "delete_traces", | ||
| workspaceId, | ||
| userName, "pairs_size=%s".formatted(batch.size())); | ||
| selectTracesMutationTable(template); | ||
|
|
||
| var projectIds = batch.stream().map(pair -> pair.getLeft().toString()).toArray(String[]::new); | ||
| var traceIds = batch.stream().map(pair -> pair.getRight().toString()).toArray(String[]::new); | ||
|
|
||
| // Prune to the batch's own partitions when every id in the batch allows it; otherwise emit the | ||
| // unbounded form. Needs no schema flag: WeeklyPartitions derives a value per id_at type the | ||
| // mutation may meet, so the set is correct on the legacy traces and on the partitioned successor. | ||
| var partitions = WeeklyPartitions.of(batch.stream().map(Pair::getRight).toList()); | ||
| // Flag only, exactly like distributed_wrap: the values reach ClickHouse via the bind below, | ||
| // never through the template, so the rendered SQL is constant regardless of batch contents. | ||
| partitions.ifPresent(_ -> template.add("partitions", true)); | ||
|
|
||
| var statement = connection.createStatement(template.render()) | ||
| .bind("workspace_id", workspaceId) | ||
| .bind("project_ids", projectIds) | ||
| .bind("trace_ids", traceIds); | ||
| .concatMap(batch -> deleteBatch(batch, workspaceId, userName, connection)) | ||
| .then()); |
There was a problem hiding this comment.
TraceDAO.delete can finish an earlier executeDelete before a later partition fails, so .block() surfaces the error while those deleted rows produce no TracesDeleted event or deferred deletion capture — should we track completed statements or add atomic recovery? TraceServiceImplTest only covers failures before any delete succeeds; should we add a distinct-partition partial-success case asserting the first row is deleted but no event or bridge rows are emitted?
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/main/java/com/comet/opik/domain/TraceDAO.java` around lines
3685-3688, refactor `TraceDAO.delete`/`deleteBatch` so a failure in a later partition
mutation cannot leave earlier successful deletions unrepresented to the cascade and
deferred deletion-event capture. Return or propagate an explicit outcome containing the
successfully deleted `(project_id, trace_id)` pairs, and update the relevant service
call path to capture/post events for those pairs even when the overall request fails;
alternatively implement a reliable recovery/retry path that completes the remaining
deletes before reporting failure. Then update or add a `TraceServiceImplTest` (or
equivalent integration test at the service/DAO seam) using IDs resolving to at least two
partitions, making the first delete succeed and the second fail, asserting that the
earlier row is deleted while no `TracesDeleted` event or bridge rows are emitted for the
failed batch.
| return makeMonoContextAware((userName, workspaceId) -> Flux | ||
| .fromIterable(Lists.partition(List.copyOf(projectIdTraceIdPairs), ANALYTICS_DELETE_BATCH_SIZE)) | ||
| .concatMap(batch -> { | ||
| var template = getSTWithLogComment(DELETE_BY_PROJECT_ID_TRACE_ID_PAIRS, "delete_traces", | ||
| workspaceId, | ||
| userName, "pairs_size=%s".formatted(batch.size())); | ||
| selectTracesMutationTable(template); | ||
|
|
||
| var projectIds = batch.stream().map(pair -> pair.getLeft().toString()).toArray(String[]::new); | ||
| var traceIds = batch.stream().map(pair -> pair.getRight().toString()).toArray(String[]::new); | ||
|
|
||
| // Prune to the batch's own partitions when every id in the batch allows it; otherwise emit the | ||
| // unbounded form. Needs no schema flag: WeeklyPartitions derives a value per id_at type the | ||
| // mutation may meet, so the set is correct on the legacy traces and on the partitioned successor. | ||
| var partitions = WeeklyPartitions.of(batch.stream().map(Pair::getRight).toList()); | ||
| // Flag only, exactly like distributed_wrap: the values reach ClickHouse via the bind below, | ||
| // never through the template, so the rendered SQL is constant regardless of batch contents. | ||
| partitions.ifPresent(_ -> template.add("partitions", true)); | ||
|
|
||
| var statement = connection.createStatement(template.render()) | ||
| .bind("workspace_id", workspaceId) | ||
| .bind("project_ids", projectIds) | ||
| .bind("trace_ids", traceIds); | ||
| .concatMap(batch -> deleteBatch(batch, workspaceId, userName, connection)) | ||
| .then()); |
There was a problem hiding this comment.
Missing multi-partition cascade coverage
The new concatMap fan-out is only covered by direct TraceDAO tests, while existing DeletionEventTest batches stay within one partition, so multi-partition deletes aren't verified end-to-end to publish one TracesDeleted cascade and capture every deletion_events_local row — should we add a service-level integration test with distinct partitions and capture enabled?
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/main/java/com/comet/opik/domain/TraceDAO.java` around lines
3685-3688, add service-level integration coverage for the `delete` method's
multi-partition `concatMap` completion behavior, rather than testing only the DAO
directly. Extend the relevant `DeletionEventTest` to create traces with IDs belonging to
distinct weekly partitions, enable deletion-event capture, and invoke deletion through
the service/API path. Assert that completion publishes exactly one expected
`TracesDeleted` cascade and that `deletion_events_local` contains every corresponding
deletion event.
| return makeMonoContextAware((userName, workspaceId) -> Flux | ||
| .fromIterable(Lists.partition(List.copyOf(projectIdTraceIdPairs), ANALYTICS_DELETE_BATCH_SIZE)) | ||
| .concatMap(batch -> { | ||
| var template = getSTWithLogComment(DELETE_BY_PROJECT_ID_TRACE_ID_PAIRS, "delete_traces", | ||
| workspaceId, | ||
| userName, "pairs_size=%s".formatted(batch.size())); | ||
| selectTracesMutationTable(template); | ||
|
|
||
| var projectIds = batch.stream().map(pair -> pair.getLeft().toString()).toArray(String[]::new); | ||
| var traceIds = batch.stream().map(pair -> pair.getRight().toString()).toArray(String[]::new); | ||
|
|
||
| // Prune to the batch's own partitions when every id in the batch allows it; otherwise emit the | ||
| // unbounded form. Needs no schema flag: WeeklyPartitions derives a value per id_at type the | ||
| // mutation may meet, so the set is correct on the legacy traces and on the partitioned successor. | ||
| var partitions = WeeklyPartitions.of(batch.stream().map(Pair::getRight).toList()); | ||
| // Flag only, exactly like distributed_wrap: the values reach ClickHouse via the bind below, | ||
| // never through the template, so the rendered SQL is constant regardless of batch contents. | ||
| partitions.ifPresent(_ -> template.add("partitions", true)); | ||
|
|
||
| var statement = connection.createStatement(template.render()) | ||
| .bind("workspace_id", workspaceId) | ||
| .bind("project_ids", projectIds) | ||
| .bind("trace_ids", traceIds); | ||
| .concatMap(batch -> deleteBatch(batch, workspaceId, userName, connection)) | ||
| .then()); |
There was a problem hiding this comment.
Repeated deletes lack throughput control
delete can submit hundreds or thousands of sequential ClickHouse mutations per POST /traces/delete request, so an authorized caller repeating maximum-size requests can overwhelm ClickHouse — should we add a delete-specific rate/usage limit or a bounded shared concurrency gate?
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/main/java/com/comet/opik/domain/TraceDAO.java around lines
3685-3688, the `delete(Set<Pair<UUID, UUID>>...)` method can submit hundreds or
thousands of ClickHouse mutations per request without any rate, usage, or concurrency
guard. Add delete-specific admission control—such as a shared bounded
semaphore/concurrency gate and an appropriate rate or usage limit on the corresponding
`POST /traces/delete` endpoint—so repeated maximum-size requests cannot overload
ClickHouse. Preserve batching behavior, reject or backpressure requests when capacity is
exhausted, and add coverage for the limit.
| var grouped = TRACES_LOCAL_TABLE.equals(table) | ||
| ? WeeklyPartitions.groupByPartition(batch.stream().map(Pair::getRight).toList()) | ||
| : Optional.<Map<Long, Set<UUID>>>empty(); |
There was a problem hiding this comment.
Deletes fail on incompatible schemas
deleteBatch emits IN PARTITION <yyyyMMdd> whenever the resolved table is traces_local, but readiness accepts any MergeTree-family table without verifying migration 000114’s partition contract, so an unpartitioned or differently partitioned table can reject the mutation or match no rows — should we fall back to the unbounded mutation or validate the live partition contract first?
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/main/java/com/comet/opik/domain/TraceDAO.java around lines
3730-3732, fix the `deleteBatch` partition-scoping gate, which assumes that
`traces_local` always has migration 000114’s weekly partition key based only on its
name. Require validation of the live table’s exact partition key/expression and type
before emitting `IN PARTITION`; otherwise fall back to the unbounded delete mutation.
Update the topology readiness check and relevant tests so an incompatible schema cannot
report healthy and trigger this branch.
| return Flux.fromIterable(grouped.get().entrySet()) | ||
| .concatMap(entry -> { | ||
| var partitionPairs = entry.getValue().stream() | ||
| .flatMap(id -> pairsById.get(id).stream()) | ||
| .toList(); | ||
| return executeDelete(table, partitionPairs, entry.getKey(), workspaceId, userName, connection); | ||
| }) |
There was a problem hiding this comment.
deleteBatch issues unbounded sequential executeDelete calls over grouped.entrySet(), so large batches can time out after only a prefix completes; retries then re-resolve ownership from live traces, leaving related rows and deletion-event bridge entries orphaned with no repair path. Should we bound or split the fan-out and durably track per-partition completion against the originally resolved set?
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/main/java/com/comet/opik/domain/TraceDAO.java around lines
3743-3749 (and the surrounding `delete` flow at lines 3685-3688), address two related
issues in the `deleteBatch` partition-mutation loop: 1. Bound the fan-out:
`grouped.entrySet()` traversal issues unbounded sequential `executeDelete` calls — the
existing pair-size limit does not bound mutation/partition count, so a batch spanning
many weeks/partitions can time out mid-way. Add an explicit request-level fan-out
budget, validate the complete planned operation before executing any mutation, and
either reject with a clear failure or split into independently bounded operations while
preserving every pair and the existing unbounded fallback behavior. 2. Make partial
failure recoverable: if a later partition mutation fails after an earlier one succeeds,
retrying should not rely on re-resolving ownership from live traces, since successfully
deleted pairs disappear from that resolution. Preserve the complete originally resolved
`(project_id, trace_id)` set across retries, or add durable per-partition
completion/cascade tracking, so related rows and deletion-event bridge entries for
already-deleted pairs get cleaned up. Add or update tests covering: (a) a batch spread
across more partitions than the configured limit, verifying no partial prefix is
submitted, and (b) a later-partition failure and retry, verifying related rows and
deletion-event bridge entries are also removed.
2 participants
Details
A ClickHouse mutation is registered against every active part of the target table before its
WHEREclause is considered at all — the existing partition predicate (OPIK_6901) prunes which rows a matched part rewrites, not which parts the mutation visits. Post-cutovertraces_localholds ~1,900 partitions / ~3,650 parts, so an ordinary delete matching a handful of rows was registered against all of them at ~19 ms/part of fixed overhead. That is what pushed trace deletes pastmax_execution_time(Code 159 TIMEOUT_EXCEEDED), where the statement errors and skips theTracesDeletedcascade and the deletion-events bridge write even though the mutation completes and the rows are in fact deleted.This scopes each statement to the partitions its own batch resolves to via
IN PARTITION, emitting one statement per partition and falling back to today's unbounded form when the batch can't be derived exactly, or when the target table isn't actually partitioned.WeeklyPartitions#groupByPartition(new) groups ids by partition rather than naming their union —IN PARTITIONaccepts exactly one partition per statement, and ClickHouse'sIN PARTITION p1, p2, …form is for composite keys, not a list of distinct partitions.traceshas no partition key at all, soIN PARTITIONagainst it is a hardCode 248 INVALID_PARTITION_VALUE, not a no-op.deleteBatchgates ontracesMutationTable()'s own resolved name — never the wrap flag directly, whichTraceMutationRoutingArchTestforbids.<if(partition)>, matching the pre-diff code's own conditional-fragment pattern rather than duplicating theWHERE/SETTINGSbody across two near-identical constants.WeeklyPartitions#of, the flat-union predecessor, is removed — it had no production callers left onceTraceDAOswitched over. Its unique boundary-case coverage was migrated togroupByPartitionrather than dropped.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
/code-reviewpasses were run against the branch and their findings triaged — fixes applied for the deadof()method, a vacuous test assertion, stale{@link}references, SQL-template duplication, and a mutable collection escapingpartitionsOf. Three findings were deliberately not fixed and are documented at their call sites instead (see Known tradeoffs below).Testing
All run locally against real ClickHouse via testcontainers (image
altinity/clickhouse-server:26.3.16.10001.altinitystable, production's exact version), on the final rebased commit:WeeklyPartitionsTestTracesPartitionPruningMutationTestTracesLegacyTablePruningMutationTestTraceMutationRoutingArchTestScenarios validated:
IN PARTITIONstatements (honest week + legacy 32-bit wrap), eachpairs_size=1.DateTime64-ceiling ids, on both the partitioned and legacy tables.ANALYTICS_DELETE_BATCH_SIZEchunks derives partitions per chunk, not once per request.Test-infrastructure fix worth flagging:
TracesPartitionPruningMutationTestpreviously asserted pruning viaEXPLAIN, which reports what the read planner selects for aSELECT— a different layer from what a mutation is registered against. That is precisely the gap this ticket exists to close, so the old coverage passed while every delete still rewrote every part. Rewritten to assert onsystem.part_logMutatePartevents. Separately,nonV7IdDisablesPruningwas asserting against a retired regex that the new SQL shape can never match, making it vacuously true; migrated to the samedoesNotContain("IN PARTITION")check its siblings use.Not run, with reason — acceptance criteria 3, 4, and 7 are unverified:
TracesDeletedcascade fires again once the statement stops erroring) and 4 (deletion_events_localreceives its row) need the full cascade path running, not reachable from these suites.These are the criteria that confirm the user-visible consequence of the fix, as distinct from the mutation-level mechanism proven above. I'd treat them as a merge gate rather than a follow-up.
Known tradeoffs — deliberately not fixed here
Both surfaced repeatedly in review; both are documented at their call sites rather than addressed, because fixing either expands beyond this ticket's stated scope:
TraceService's delete→cascade coupling is still all-or-nothing. A batch can now complete several partitions' statements before one errors, so more of a batch's rows can be synchronously, confirmedly deleted without the cascade firing than before this change. Deletes are idempotent, and the ticket is explicit: "removes the condition that trips it; does not restructure the coupling." Scoping each mutation so it completes well inside the timeout is what makes hitting this much rarer.IN PARTITIONgate is a name check, not a live schema read. AtracesDistributedWrapEnabled()flip that precedes the EXCHANGE would turn deletes intoCode 248rather than degrading to unpruned-but-correct. This is the same operational preconditiontracesMutationTable()already relies on for every other mutation in the class (a premature flip breaks reads and inserts too, just differently) — made explicit rather than newly introduced.Documentation
No documentation update required — internal query-shape change with no user-facing API, schema, or behavioural surface. Rationale that would otherwise live in docs is captured in the
TraceDAOandWeeklyPartitionsJavadoc, including whyIN PARTITIONis interpolated rather than bound, whyDELETE FROMis kept over a hand-writtenALTER … UPDATE, and why the far-future double-partition case is safe.