[OPIK-6901] [BE] perf: prune trace deletes to the batch's own partitions - #7912
Open
thiagohora wants to merge 38 commits into
Open
[OPIK-6901] [BE] perf: prune trace deletes to the batch's own partitions#7912thiagohora wants to merge 38 commits into
thiagohora wants to merge 38 commits into
Conversation
Deleting traces rewrote every part of the table. A mutation selects parts at the PARTITION
stage, and DELETE_BY_PROJECT_ID_TRACE_ID_PAIRS constrains only (workspace_id, project_id, id) --
none of which the weekly partition expression is derived from -- so nothing is pruned.
Measured on prod-test (traces_local, 271.6 M rows, 3,928 parts, post-cutover):
EXPLAIN indexes=1, 3 ids in one week
current predicate partition stage: Parts 3928/3928 (Condition: true)
+ partition-expression set (this change) partition stage: Parts 5/3928
a real delete of 12 ids
3,928 parts per replica rewritten, 5.40 TiB, to mask 12 rows
An id_at RANGE is not a substitute. On a batch spanning 1996 and 2200 a range still selected
2,644 of 3,928 parts, because the range covers every week in between; the exact set selected 4.
Scattered batches are the normal case for a delete-by-ids API, so the predicate is a set.
The predicate is emitted ONLY when every id in the batch is a UUIDv7, which preserves the
guarantee the original javadoc called out: a row whose id_at cannot be trusted is still deleted,
because no id in such a batch is used to derive a partition. All-or-nothing per batch -- the SQL
is either fully pruned or byte-identical to the previous unbounded form, never partially bounded.
Deriving a partition from a non-v7 id would read whatever bits sit in the timestamp field, and a
wrong partition is a SILENTLY skipped delete, which is worse than a slow one.
Far-future ids are deliberately supported rather than excluded: their id_at is bogus but
self-consistent, so they live in the far-future partition this computes. Verified on prod-test:
0 partition mismatches between toMonday(id_at) and toMonday(UUIDv7ToDateTime(id)) across all
11.23 M far-future rows (4.1% of the table).
DELETE_FOR_RETENTION already carries equivalent bounds for the same reason (OPIK-6900); this
brings the by-ids path in line, with the extra v7 guard its arbitrary id set requires.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
Contributor
⏱️ pre-commit per-hook timing
⏭️ 41 skipped (no matching files changed)
|
Collaborator
|
No test needed here. Nothing for e2e to reach 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 20 Aug 16:11 UTC — nothing the verdict depends on changed. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
…com.comet.opik.utils The Monday-of-week derivation is a pure function of a UUID, and the same partition expression backs both traces_local_v2 and spans_local_v2, so it does not belong to TraceDAOImpl. Moved as-is to com.comet.opik.utils.WeeklyPartitions, following SentinelTranslation: a @UtilityClass whose javadoc carries the reasoning the callers must not re-derive - above all that an empty result means "emit no predicate", not "no partitions". Pure move; no behaviour change. The test moves with it as com.comet.opik.utils.WeeklyPartitionsTest. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…och" The id encodes 1996-02-09, which is 26 years AFTER the Unix epoch, so the label was simply wrong - and misleadingly so, since it implied coverage of a negative id_at that a UUIDv7 cannot carry (the 48-bit timestamp field is unsigned). Renamed to say which year it is, and the comment now states the two facts the case actually rests on: after the epoch, and inside Date32's 1900 floor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… past DateTime64's ceiling Java LocalDate has no upper bound; id_at does. DateTime64 spans [1900-01-01, 2299-12-31 23:59:59.99999999] and SATURATES there rather than wrapping or throwing, so a UUIDv7 whose embedded timestamp is past that instant is stored as 2299-12-31 23:59:59 and filed under partition 22991225 - while the derivation computed its honest, out-of-range week. That is a partition the row is not in, and the predicate would then match zero rows and report success: the exact silent-skip failure the all-or- nothing v7 guard exists to prevent, reached by a different route. Rejecting rather than clamping to 22991225. Clamping would make correctness depend on reproducing ClickHouse's saturation semantics exactly - and there are two saturating steps before toDate32 is even reached, since UUIDv7ToDateTime returns DateTime64(3) and the column is DateTime64(0) - to buy pruning for ids that should not exist. Returning empty costs performance on those batches and nothing else. Only the ceiling is guarded, deliberately: `>>> 16` reads the timestamp field unsigned, so the value is in [0, 2^48) and the earliest id_at any UUIDv7 can carry is the epoch, whose Monday (1969-12-29) is 70 years above Date32's 1900 floor. A below-1900 id_at is unreachable by construction, not merely untested, and the same bound is why Instant.ofEpochMilli cannot overflow here. Tests cover both ends and the boundary millisecond either side of the ceiling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…artitioned successor
The template still targets the LEGACY traces when the wrap is off, and that
table is the one place the predicate must never appear. It has no PARTITION
BY at all - one `all` partition, so there is nothing to prune - and it
declares id_at as a 32-bit DateTime('UTC') that overflows past 2106
(migration 000091). A far-future UUIDv7 - the litellm ~2201 ids, real
customer-facing rows - is therefore stored under a WRAPPED recent
timestamp, which the derived partition cannot match, so the delete matches
zero rows and reports success. Pure downside: no pruning to gain, a silent
data bug to lose.
Gated on a NEW flag, tracesWeeklyPartitioningEnabled, because neither
existing schema flag marks the EXCHANGE - the moment the partitioning
appears - and each is wrong in a different direction:
* tracesDistributedWrapEnabled is too LATE. The wrap is a separate,
deferrable step (--skip-wrap now, --wrap-only weeks later), so between
the two `traces` is already the successor while the flag is still
false. That is the state prod-test sat in for ~30 minutes today.
Gating on it would merely forgo the pruning there.
* traceColumnsNonNullable is too EARLY, which is the dangerous
direction. It is a runtime concern, not a schema one - the suite
TraceSentinelIntegrationTest sets it true against the unpartitioned
legacy table on purpose - and the runbook requires it rolled out
BEFORE the EXCHANGE, since a rolling restart cannot be atomic with a
metadata swap. Gating on it would emit the predicate against the
legacy table for the whole rollout window.
The flag asserts a schema fact: id_at as DateTime64(0,'UTC') under the
weekly PARTITION BY. Unlike its two siblings it is safe to LAG and unsafe
to LEAD - false is the previous unbounded mutation, always correct and
merely slower - so it is turned on at leisure once the EXCHANGE is
confirmed, and must be reverted (with the restart) BEFORE a rollback
promotes the original. Documented as such in the cutover runbook, both
where the other flips are described and in the rollback section.
Plumbed like tracesDistributedWrapEnabled: config.yml + config-test.yml,
docker-compose, helm values/configmap/README, and the configmap env test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ind against ClickHouse
WeeklyPartitionsTest covers only the Java derivation, so nothing failed if
the template stopped rendering the predicate, if `partitions` was bound
under a different name, or if ClickHouse rejected a Long[] on
`IN :partitions` - and the last of those had never actually been executed:
the prod-test measurement that motivated this change used EXPLAIN with a
literal set, not the bound driver path.
TracesPartitionPruningMutationTest closes that, against the post-EXCHANGE
topology (dedicated non-reused containers, since the EXCHANGE destructively
swaps `traces`). Every case asserts BOTH the rows read back through the
public API and the SQL ClickHouse received, read from system.query_log by
log_comment - because either alone passes for the wrong reason. Rows alone
cannot see pruning silently stop (the delete still works, just slowly), and
SQL alone cannot see a predicate that names a partition the row is not in.
* all-v7: the predicate is emitted with the target's own partition, the
target goes, a bystander in the same project stays.
* two-week batch: both partitions in one Long[] - the multi-value bind a
single-id delete never reaches.
* non-v7 and beyond-2299: no predicate at all, and the v7 row batched
alongside is still deleted. Driven through TraceDAO directly, since
ingestion rejects both id shapes by design.
liveTracesIsTheWeeklyPartitionedSuccessor is load-bearing, not decoration:
the predicate is harmless against an unpartitioned table for recent ids, so
without it a failed EXCHANGE would leave every test above green while
proving nothing. It pins both facts the new flag asserts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
documentation
Contributor
Backend Tests - Integration Group 14 33 files + 4 33 suites +4 6m 3s ⏱️ +11s Results for commit 64b0001. ± Comparison against base commit fa282b0. This pull request removes 32 and adds 70 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…prunable `of` walked the collection straight into an enhanced for, so a null batch threw NPE from inside the loop - after the javadoc had promised an empty-result fallback for anything it cannot derive. @nonnull rather than normalising null to Optional.empty(), for two reasons. It is the prevailing convention: every other collection-taking public static method in com.comet.opik.utils is annotated that way (EnrichmentUtils.buildFeedbackScoresNode/buildCommentsNode, PaginationUtils.paginate). And normalising would be actively worse here, because empty is not an error signal - it is the documented answer "this batch cannot be pruned, emit the unbounded form". A caller that lost its batch would receive that answer, issue a correct-but-unbounded mutation, and never learn it had a bug. A null collection is a programming error; a null ELEMENT is the data condition, and that keeps returning empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reproduced: the returned Optional wrapped the working HashSet, so
`of(...).orElseThrow().add(99L)` succeeded. Now Set.copyOf, per
apps/opik-backend/AGENTS.md ("prefer immutable collections").
This is more than collection hygiene because of what the set is. It is the
exact list of partitions a trace DELETE binds to `IN :partitions`, and the
partitions a batch resolves to are the ONLY places its rows can be - that is
the entire premise of the pruning. A caller that removed an entry would not
get a slower delete, it would get one that matches nothing and reports
success: the same silent-skip failure the v7 guard and the DateTime64
ceiling guard exist to prevent, reached through the return value instead.
The accumulator stays a local HashSet - copyOf is taken once, at the
boundary - so dedup across a batch is unaffected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tracesWeeklyPartitioningEnabled read as "create/activate weekly partitioning". It does no such thing: it enables partition-aware PRUNING of trace deletes, and the partitioned schema arrives with the cutover's EXCHANGE and nowhere else. Not cosmetic, given what this particular flag is. It is documented as safe to LAG and unsafe to LEAD, so the name actively invited the one mistake the docs warn against - an operator reading it as a step that makes the cutover progress, setting it early to "turn partitioning on", and instead starting to emit a partition predicate against the legacy unpartitioned `traces`, where a far-future id then matches zero rows and reports success. Renamed everywhere in one pass - 21 references across 11 files: config.yml, config-test.yml, DatabaseAnalyticsDataModelConfig (record component + javadoc), TraceDAO (accessor + javadocs), the cutover runbook (4), docker-compose env var + comment, helm values/configmap/README/env test, and the integration test's CustomConfig. Verified no occurrence of the old name or env var remains. Env var likewise renamed (ANALYTICS_DB_DATA_MODEL_TRACES_WEEKLY_PARTITION_PRUNING_ENABLED); it is not yet set in any deployment repo, so nothing external breaks. Also reworded the descriptions to lead with what the flag does rather than what must already be true - "enables pruning; it does NOT create or activate any partitioning; turning it on ASSERTS a schema fact rather than causing one" - since a correct name with prose that still opens "whether the live table is partitioned" would re-create the ambiguity. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ubstring The live-schema guard accepted any partition_key containing toYYYYMMDD, toDate32(id_at), toIntervalDay and toDayOfWeek(id_at). A guard whose whole job is catching drift should not be satisfiable by an expression that merely uses the right functions - `toMonday(id_at)` plus those names in any arrangement would have passed. Replaced with two exact pins, because the one thing that has to hold is stated in three independently-maintained places (the migration's PARTITION BY, the DAO's predicate, and WeeklyPartitions.of) and drift in any one silently skips deletes. 1. Value agreement, parameterized over three eras. Seeds a row per era and asserts _partition_id (where ClickHouse actually filed it, i.e. the migration as installed) == the DAO predicate evaluated on that row == WeeklyPartitions.of. Follows the existing _partition_id idiom in TracesLocalV2TableTest, where the partition id is that Monday's YYYYMMDD. The eras are load-bearing: toMonday agrees with the Date32 expression across the ordinary calendar and diverges only far-future or at the epoch, so a recent-only sample would accept the very expression 000114 was written to escape. The far-future row is what makes it bite. Rows are seeded in raw SQL (ingestion rejects backdated and far-future ids by design) supplying only the three columns without a DEFAULT, so id_at comes from the real MATERIALIZED definition rather than a restated copy - restating it would add the drift surface this test is for. 2. AST agreement, which value agreement does NOT give. Pruning requires the planner to recognise the predicate as being on the partition key expression, so an equal-valued but differently-written expression would leave every delete correct while quietly rewriting every part again - this PR's exact regression, invisible to every other assertion in the suite. Asserted by round-tripping the DAO's text through ClickHouse as a partition key of its own and diffing the two re-prints: both come from the same printer, so they match iff the parsed expressions do. That is formatter-independent by construction, which diffing the DAO text against system.tables directly would not be - and pinning ClickHouse's whitespace is what the guard must not do. The id_at DateTime64 half moves to its own test; it is not implied by either pin, since a 32-bit id_at agrees with itself while wrapping every id past 2106. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d test
ordinaryId, farFutureId and oldId ran the same one-line assertion over
different data, so they are now one @ParameterizedTest keyed by era
("ordinary id" / "id from 1996" / "far-future id" as the display name, so a
failure still says which one).
Kept deliberately: the ClickHouse-derived expected values and the note that
they are what prod-test actually returned rather than hand-computed Mondays
- that provenance is the whole point of these assertions. The @MethodSource
javadoc now also records why the three eras are not interchangeable
samples: an expression correct for the ordinary calendar and wrong at the
extremes is the toMonday trap 000114 was written to escape, so dropping one
would weaken the pin rather than tidy it.
The other cases stay separate @test methods on purpose - each asserts a
different outcome (empty, throws, immutable) with its own reasoning, so
parameterizing them would trade explanation for a table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t through test SQL
Net -162 lines. The suite had grown five queries of its own to validate the
partition expression: a SELECT that re-evaluated the DAO's predicate, a
throwaway table created purely so ClickHouse would re-print that predicate
as a partition key, and two system-table lookups. That is the test
re-implementing what it is supposed to be checking, and it is the wrong
instrument: what matters is the statement production runs.
Replaced all of it with one behavioural test. Seed a row per era (recent,
1996, far-future 2200), delete them in one DAO batch, assert the rows are
gone. If the predicate resolved to any partition other than the one
ClickHouse filed a row under, the mutation would select the wrong parts and
that row would SURVIVE - so "every row is gone" IS the three-way agreement
between the migration's PARTITION BY as installed, the DAO's predicate and
WeeklyPartitions.of, established by the real delete instead of by SQL
written here. It subsumes what it replaces:
* cross-era correctness, including the toMonday trap 000114 warns about -
that expression diverges only far-future, so the 2200 row catches it
* the DateTime64 half of the flag's assertion, with no system.columns
lookup: against a 32-bit id_at the 2200 row is stored under a wrapped
timestamp, the derived partition misses it, and it survives
* the multi-value Long[] bind, and the exact-set assertion on it
Dropped with it: daoPredicateIsTheSameExpressionAsTheLivePartitionKey and
idAtIsTheSixtyFourBitColumn, the probe table and both renderers. One
guarantee goes with them and is worth naming: AST identity between the
predicate and the partition key, which is what proved the planner PRUNES
rather than merely computing the right answer. The exact-predicate text is
still checked against the emitted statement, and an over-broad bound set
still fails, so a silent loss of pruning is still caught - but a rewrite
that is semantically identical and textually different would now pass.
The suite's SQL is down to three plain text blocks with only :placeholders -
seed a row, count a row, read a statement from query_log. No StringTemplate
fragments, no interpolation, so TemplateUtils is no longer needed here.
Also renamed exchangeTables() to installPartitionedSuccessorUnderTraces()
and documented why it exists: it is setup and nothing asserts on it. The
DAO names its target table, and after the migrations the live `traces` is
the legacy unpartitioned one while the successor is only the unreachable
traces_local_v2 - so without those two statements these tests would run
against the one table where this predicate must never be emitted, and would
pass while proving nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…table helpers 61c699d broke the build - all 18 Backend Tests jobs, including Unit Tests. Removing the probe-table renderers took the two queryOneString overloads with them (they sat between partitionKeyOf and execute in the block I cut), while lastTraceDeleteSql and liveRowCount still call it. Restored as a single binder-taking overload; the no-arg convenience form had no remaining callers. Every read in the suite is a single scalar, so that is the only mapper it needs. Why local javac missed it: I was filtering "cannot find symbol" out of javac's output as dependency-resolution noise, which is exactly what this error looks like. Added a check that cross-references declared private members against call sites and method references instead, which reports both a call with no declaration and a declaration with no callers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ether the rows go
Restores, in a better instrument, the property the removed AST pin covered.
Correctness and pruning are different claims and the suite only made the
first: deletes were already correct before this change, so a suite that
cannot see pruning stop does not test what the change exists to do. The
guarded regression is specific - a migration rewrites the partition
expression to something semantically identical but textually different, the
planner stops recognising the DAO's predicate as the partition key, pruning
silently stops, values still agree, every row is still deleted, and every
other assertion here stays green.
EXPLAIN indexes = 1, json = 1, following TracesLocalV2PartitioningTest's
idiom and record shape rather than inventing one. EXPLAIN does not accept a
mutation, so the WHERE clause is lifted verbatim out of the DAO's OWN
emitted DELETE - predicate and inlined partition values included - and put
behind a SELECT. Only the verb changes; the statement explained is still the
DAO's, so this does not reintroduce a test-authored copy of the predicate.
Verified the lift standalone against pruned, unbounded and post-wrap
(traces_local) statement shapes.
Asserts both directions, since a pruning assertion that would also pass
without pruning is worth nothing - the same trap as `.contains(partition)`
and `doesNotContain("toDayOfWeek")`:
* bounded delete: selected parts strictly fewer than initial. No
hard-coded 5; the comparison is against what the table holds, and the
test seeds three eras first so there are several partitions to prune.
* fallback delete: prunes nothing - either no partition index at all,
because nothing filters the key, or every part still selected.
One subtlety that would have made the fallback look pruned: PrimaryKey is
deliberately excluded from the entries considered. The DAO's WHERE also
filters workspace_id and (project_id, id), which are the sort key, so
PrimaryKey prunes parts for the unbounded statement too; counting it would
have destroyed the discrimination. Only MinMax and Partition entries count,
and across them it takes the smallest selected and largest initial so it
does not depend on which one reports the pruning.
Not verified: this against ClickHouse. Which of MinMax/Partition carries
the numbers, and whether the fallback yields an entry at all, are read from
ClickHouse's documented behaviour rather than observed - hence tolerating
both shapes instead of pinning one. CI is the check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Reviewer: a false-flag regression during rollback passes unnoticed. Correct, and the gap is wider than rollback - flag off against legacy `traces` is the state EVERY deployment is in today. Nothing caught it. Every other trace-delete suite runs in exactly that state and asserts only that rows go away - which they do for a RECENT id, because the legacy 32-bit id_at is accurate for one. The damage shows only on a far-future id (litellm ~2201): the legacy column stores a wrapped recent timestamp, a derived partition cannot match it, and the delete reports success having matched ZERO rows. No existing test has such a row, because ingestion rejects far-future ids by design. TracesPruningDisabledMutationTest seeds one and asserts the emitted SQL carries no partition predicate and no id_at narrowing of any kind, and that the row is deleted. Shared containers - it changes no topology. The project comes from the real ingestion path (create a trace through the endpoint, read the project id back off it) rather than a fabricated identifier, and the test asserts that id is a UUIDv7. Only the far-future row is raw, because the 24h ingestion window refuses it; a recent id would prove nothing here anyway, since even a wrongly-emitted predicate matches one. Self-skips once the cutover migration lands: at that point there is no legacy `traces` and the wrapping hazard stops existing, so an assumption in beforeAll reports the reason rather than failing on a premise that has legitimately gone away. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d, and survive the migration Two problems with the pruning suite's data, both fabrication rather than fixtures. `UUID.randomUUID()` is a v4, and project ids are UUIDv7 - the backend mints them through IdGenerator. So the era and EXPLAIN tests were deleting from a project id that could not exist, in a project that did not exist, in a PR whose whole subject is that the id version matters. The project now comes from the real ingestion path: create a trace through the endpoint, read the project id back off it, and assert it is a v7. The recent row now goes through the endpoint too. Only the 1996 and 2200 eras stay raw, because the ingestion window is 24h and no endpoint can create them; they are seeded into the project the endpoint just made. (WORKSPACE_ID and API_KEY stay random UUIDs - workspace_id is a String column, not an entity id, and 62 suites in this tree do the same.) Separately, the setup was pinned to today's estate: it EXCHANGEd `traces` with `traces_local_v2` unconditionally, so once the cutover migration lands - `traces` already the successor, `traces_local_v2` gone - beforeAll would die on a table that no longer exists. Now idempotent: if `traces` already partitions on id_at it does nothing, and if neither state holds it says so instead of surfacing a bare "table not found". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…covering both flags on Better than what it replaces, and it withdraws a decline I made earlier. The suite was reaching the partitioned data by renaming tables under the DAO and leaving it on the <else>traces branch. It now applies the wrap and sets tracesDistributedWrapEnabled, so the DAO's mutations reach the data through the configuration switch that governs them in production - DELETE FROM traces_local, chosen by the flag - while traces is the Distributed wrapper that reads and inserts flow through. That is why the endpoint-created row and the raw-seeded ones land in the same place. This is the post-cutover end state, and it covers both schema flags on at once. I declined that in an earlier round as needing a third topology and its own suite; that was wrong - the wrap flag is how the DAO is pointed at the data, so enabling it costs two setup statements and covers the combination the fleet actually ends up in. The javadoc note claiming the cell was untested is replaced rather than left to mislead. distributedTracesRejectsDirectMutation keeps the claim from being vacuous: had the wrap not taken effect, traces would still be a MergeTree, every pruned delete here would have run against it, and nothing would have said so. It asserts the specific ClickHouse rejection, as the wrap suite does. What is no longer covered here is the transient post-EXCHANGE/pre-wrap window. The predicate is identical in both - traces_local is the same physical table under another name - and the flag's javadoc already records that it must hold in that window. Both setup steps are idempotent, so this survives the cutover migration: the successor install skips when traces_local already exists or traces already partitions on id_at, and the wrap skips when traces is already Distributed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…onally Both components are ints, so `new SelectedParts(min, max)` compiles fine after a field-order change and silently inverts selected/total - which would invert the pruning assertion itself, the one thing that test exists to check. .agents/skills/opik-backend/SKILL.md is explicit: "Always annotate records/DTOs with @builder(toBuilder = true)", "Use builders (not constructors) when instantiating records", and its BAD example is a positional `new`. The sibling record in TracesLocalV2PartitioningTest already carries the annotation; I left it off deliberately as unnecessary, which was wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
Backend Tests - Integration Group 5397 tests 392 ✅ 3m 40s ⏱️ Results for commit ad26315. ♻️ This comment has been updated with latest results. |
Two defects in b31f186, both mine, both in the setup rather than the assertions. 1. NPE before the diagnostic. queryOneString returns null when no row matches, so partitionKeyOf("traces").contains(...) threw if `traces` was absent - which is a reachable state, since a wrap interrupted between its CREATE and its RENAME leaves `traces` renamed away. The guard existed precisely to report an unsupported topology and would instead have died dereferencing it. partitionKeyOf now returns "" for a missing table, and the assertion message quotes the partition key it did find. 2. The wrap was not re-entrant. It returned early when `traces` was already Distributed, but if a run died between CREATE traces_dist and the RENAME, the leftover wrapper made the next CREATE fail on a duplicate name and buried the real state. Now drops any stranded traces_dist first - it holds no data, being a routing definition - which is the same reset TracesLocalV2CutoverTest performs. Dedicated containers mean neither state can survive into a fresh run today, so this is defence rather than a live bug. It is still worth having: the first one turns a diagnostic into an NPE, and both would only ever be hit while someone was debugging a broken setup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… hardening 84304a7 appended a sentence to ensureDistributedWrap's javadoc as a continuation line with a single leading space, where every other line in that block has five. Spotless's indentation pass rewrote it, so Code Quality went red on one line. Third formatting round-trip in this review, so I added a check for the class of mistake rather than just the instance: it flags block-comment continuation lines whose '*' indent is not a legal javadoc alignment (1, 5, 9, ...). Both test suites come back clean. It has false positives on '*' inside SQL text blocks - the eight it reports in TraceDAO are column lists in SELECT statements, none of them touched by this branch - so it is a pre-push aid for changed files, not a gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…uites do
Read TracesLocalV2PartitioningTest / SpansLocalV2PartitioningTest properly
and my data layer was the outlier: magic constants where the neighbours are
arithmetic.
They mint ids from a named, fixed anchor - ID_GENERATOR.generateId(instant)
off ANCHOR_MONDAY plus week offsets - and get a real v7 project id straight
from ID_GENERATOR.generateId(). Mine hard-coded opaque UUID literals
("00bfd451-fa93-7c10-..." with a comment claiming "id_at 1996-02-09") paired
with hard-coded partition literals (19960205L). Both sides magic, the
relationship between them unverifiable by a reader, and the comment the only
thing asserting what the id even was.
Now: three ERA_MONDAYS named as dates, ids minted mid-week from them by
idInWeekOf so the assertion exercises the map back to Monday rather than
identity, and expectations from partitionNameOf on the same Mondays - which
formats a Monday the test already names rather than re-deriving "the Monday
of an arbitrary date", the part actually under test. Fixed rather than
now-derived for the reason the sibling suite documents: the math stays
deterministic and cannot drift across a week boundary mid-suite.
Zero UUID.fromString literals remain. The other two ids are now
self-evident too: NON_V7_ID is UUID.randomUUID(), a v4 by definition, and
OUT_OF_RANGE_ID is minted one instant past the DateTime64 ceiling so the
boundary it sits past is visible.
Dropped the endpoint round-trip from the era and EXPLAIN tests: I had added
it partly to obtain a genuine v7 project id, which ID_GENERATOR.generateId()
gives directly - as the sibling suites show. The endpoint stays where it
earns its keep, in the live-user-path delete and the fallback test.
Verified the load-bearing assumption by execution: all three ERA_MONDAYS are
actual Mondays, a mid-week minted id derives back to each, and both
underivable shapes are rejected.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…QL in the EXPLAIN helper Two reviewer findings, plus the spotless leftover from 01a7c1e. 1. LIVE_ROW_COUNT omitted project_id while TraceDAO.delete matches on (workspace_id, project_id, id), so the oracle asked a narrower question than the delete answers: a row for the same id in another project keeps the count at 1 after a successful delete, failing a test whose subject worked. Given how much of both suites rests on "the row is gone", that is a wrong oracle rather than a tidiness issue. Predicate added and project_id bound at every call site in both suites - the sibling one matters more, since it runs on shared containers. 2. partsSelectedBy built its SELECT and EXPLAIN with .formatted(), splicing two regex groups lifted out of query_log into executable SQL. The rule names that construct explicitly ("No +, no String.format / .formatted(...)"), and its table gives the way out: fragments through StringTemplate, values bound. So the EXPLAIN is now a declared text block with <table> and <partition_expression> as fragments and :partitions bound. Both constraints turn out to be satisfiable at once, so the instruction to lift the predicate from the DAO rather than re-author it does not need revisiting. The predicate fragment is PARTITION_PREDICATE, and using the constant is not re-authoring: every caller has already asserted the emitted statement CONTAINS that exact text, so it is pinned to the DAO's SQL by assertion instead of by string surgery on it - which is the stronger link, since the regex silently accepted whatever it captured. The partition values still come from the emitted statement, parsed and then bound. The DAO's workspace_id and (project_id, id) predicates are deliberately not reproduced: they are sort-key filters, cannot change partition selection, and omitting them makes the unbounded case a full scan, which is the conservative direction for asserting the fallback prunes nothing. Also converted the wrap DDL's .formatted(DATABASE_NAME) to a StringTemplate fragment. The sibling suites still splice it, but the rule says not to add new ones and I added that one two commits ago. No SQL in either suite is built by string operations now. Removed the unused java.time.Instant import spotless flagged, and added an unused-import check to the pre-push battery - third distinct formatting class to bite me, and javac cannot see it here because most types are unresolvable offline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mats
`Set.<Long> of()` — spotless wants no space after the witness. Declaring the
variable's type instead removes the need for the witness at all, so there is
nothing left to disagree about:
Set<Long> bound = ... ? boundPartitionsOf(daoDeleteSql) : Set.of();
Fourth formatting round-trip. My local battery now covers line length,
javadoc continuation indent and unused imports, but not intra-expression
spacing, which needs the real formatter — and mvn cannot read this POM
offline, so CI stays the only authority for that class.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…t documents Inserting CREATE_DISTRIBUTED_WRAPPER in 89ba903 landed it between EXPLAIN_SELECTED_PARTS's javadoc and the constant itself, so Java bound that javadoc to the wrapper - which already had its own - and left EXPLAIN_SELECTED_PARTS undocumented. The fragment/bind contract, the argument that using PARTITION_PREDICATE is not re-authoring, and the note on the deliberately omitted sort-key predicates were all still in the file, attached to the wrong declaration. Moved the wrapper block after EXPLAIN_SELECTED_PARTS so each javadoc sits immediately before the constant it describes. No text changed. Added a check for the class: a javadoc whose next non-blank line opens another javadoc documents nothing. Fifth distinct formatting/structure class this review, and the one most likely to pass every other check I have - spotless does not reflow comments, and javac does not care which declaration a javadoc lands on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…l as on The suite only ever ran with pruning enabled, so nothing in it isolated the flag as the cause of the pruning. Removal was caught - the emitted-SQL and EXPLAIN assertions fail without the predicate - but removing the flag GATE, so pruning always happens, was only caught in a different suite that also changes the schema, and so could not attribute anything to the flag alone. Restructured into two @nested classes sharing the outer class's containers and topology, following HealthCheckIntegrationTest's pattern (one app per nested class, one @registerApp each). tracesWeeklyPartitionPruningEnabled is the ONLY thing that differs between them; traceColumnsNonNullable and tracesDistributedWrapEnabled are fixed on in both, as production runs them post-cutover. * PruningEnabled - the existing five tests. * PruningDisabled - the same fixtures, same post-cutover topology: no partition predicate and no id_at narrowing is emitted, and the planner selects every part. Both are the inverse of an assertion in the enabled class, on identical data. So the pair is a control rather than two unrelated suites: delete the pruning and PruningEnabled fails; delete the flag gate and PruningDisabled fails. Correctness is unaffected either way - the rows go away in both, which is what makes it an optimisation - so only these assertions can tell the two states apart. Topology setup and every raw read now run through a container-derived TransactionTemplateAsync rather than an app-injected one: the topology has to be installed once, before either app boots. That is also the sibling partition suites' idiom. Added a type-import check to the pre-push battery. The restructure introduced a field whose type had no import - a compile error that offline javac cannot distinguish from ordinary unresolvable-dependency noise, and which my existing checks missed. It needed text-block stripping to be usable at all, since SQL keywords read as type names otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e left Lifting the tests into nested classes left two blank lines where the old afterAll used to end. Spotless collapses them. Added the check, which is the cheapest of the lot and should have been there first: consecutive blank lines are a guaranteed round-trip, since spotless always collapses them and nothing else I run looks at them. Seven pre-push checks now - line length, javadoc indent, orphaned javadoc, unused imports, unimported types, self-referential symbols, blank-line runs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…O deletes through a raw container handle
Two reviewer findings, and the second one exposed a defect of mine.
1. ensureDistributedWrap returned on engine == 'Distributed' alone, so a
wrapper over another database or another local table would satisfy the
guard, block the rebuild, and route reads and inserts away from the table
the assertions then inspect. Now checks engine_full for this database and
'traces_local'. Validates and fails rather than dropping and recreating: a
byte-compare of the whole engine expression would pin ClickHouse's
re-printing, and silently repairing a wrapper someone else left behind
hides the surprise instead of reporting it.
2. Multi-chunk pruning was untested. ANALYTICS_DELETE_BATCH_SIZE is 10000 and
the derivation sits inside the concatMap, so all-or-nothing is a
PER-STATEMENT guarantee, not per-request - a chunk with an underivable id
loses its pruning while its siblings keep theirs. Every prior test passed
one chunk's worth of pairs and could not see that. New test sends 10,002:
chunk one all-derivable, chunk two carrying NON_V7_ID, asserting both
chunks' real rows are deleted and that chunk two emitted no id_at
predicate.
Writing it surfaced two things worth more than the test:
* MY DEFECT. It died on "Code: 62. Max query size exceeded ... position
262122". ClickHouseContainerUtils.newDatabaseAnalyticsFactory sets no
queryParameters, while config.yml and config-test.yml both carry
custom_http_params=max_query_size=100000000. Moving this suite's template
to a container-derived handle for the nested-class restructure routed the
DAO's deletes through it, so every delete here ran on non-production
connection settings - confirmed on the live container: max_query_size =
262144, the default. A full chunk inlines to ~762 KiB and dies at 256
KiB. Now split: appTemplate (app-injected) for anything the DAO executes,
template (container-derived) only for the pre-app topology install and
this suite's raw seeds and reads, with javadoc on both saying they are
not interchangeable.
* A LIMIT ON THE ORACLE. With that fixed the statement ran, and the test
then failed asserting the first chunk's predicate - because
log_queries_cut_to_length is 100000 and a 762 KiB statement is truncated
in query_log long before the predicate at its tail. That assertion was on
a string the server never kept, so it is gone and the helper's javadoc
now records the limit. A derivable chunk's pruning is covered by the
single-chunk tests; what only this test shows is that the chunks are
derived independently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…GE is not needed at all The setup was already idempotent, but nothing exercised the branch that makes it so. In today's estate a fresh container always takes the INSTALL path in both steps, so both early returns are dead code - and they are precisely the path that becomes the ONLY path once the cutover migration lands and the migrations hand us `traces_local` partitioned with the Distributed `traces` over it. The EXCHANGE stops being needed then, and the first day that code matters is the wrong day to discover it was wrong. topologySetupIsANoOpOnceTheEstateProvidesIt re-runs both steps against the topology they already installed, which is that same shape: `traces_local` exists and `traces` is Distributed over it. It asserts the wrapper and the partitioned data are untouched, and then that a pruned delete still works - so a no-op setup cannot quietly leave the suite asserting against something no longer partitioned. Two states are covered now: WITH the swap, which every other test needs today, and WITHOUT it, which is the estate after the cutover. Not a tautology, and the comment says why: a step that failed to early-return would THROW rather than quietly repeat. The EXCHANGE needs `traces_local_v2`, which the install renamed to `traces_pre_cutover_backup`; the wrap ends in a RENAME onto `traces_local`, which by then exists. The class javadoc's idempotency claim now names this test instead of asserting it in prose. Verified locally: 10 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two reviewer findings, both of them the same defect I have been fixing elsewhere in this review: an assertion that would pass whether or not the behaviour it names exists. 1. The multi-chunk test asserted nothing about the first chunk, so hoisting weeklyPartitionsFor over the request would leave BOTH statements unbounded, delete the rows anyway, and still satisfy chunk two's "no predicate" assertions. My own comment claimed only this test would notice that refactor, which was false. Fixed by inverting the arrangement rather than adding an assertion, because the first chunk can never be inspected: Lists.partition sizes chunks [BATCH_SIZE, remainder], so the first is always full, and query_log truncates at log_queries_cut_to_length (100,000 bytes) while a 10,000-pair statement inlines to ~762 KiB - its tail, where the predicate sits, is never recorded. EXPLAIN cannot help either, since it reads the same truncated text. So the non-v7 id goes in the first chunk and the derivable ids in the readable remainder, which is asserted to carry the predicate and to be bounded to exactly its own two weeks. Verified by mutation test, not by argument: patching the DAO to derive over projectIdTraceIdPairs instead of batch - precisely the refactor in question - turns the test red on the named assertion, and reverting turns it green again. 2. The idempotence test captured only engine_full and partition_key, then seeded its row AFTER both helpers ran. A table recreated from the same DDL reports identical metadata while being empty, so a step that rebuilt the topology instead of skipping it satisfied every assertion, and the row inserted afterwards landed happily in the fresh table. The test could not tell "skipped" from "rebuilt" - the one thing it exists to check. Now a row is created through the ingestion path BEFORE the setup re-runs and asserted still visible afterwards via traceIdsOf, the production getTraces path. Reading it back that way also shows the Distributed wrapper still ROUTES, which no metadata check can establish and which is what a misrouted rebuild would break. Verified locally: 10 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 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
Deleting traces rewrites every part of the table. A mutation selects parts at the partition stage, and
DELETE_BY_PROJECT_ID_TRACE_ID_PAIRSconstrains only(workspace_id, project_id, id)— none of which the weekly partition expression is derived from — so nothing prunes.Measured on prod-test (
traces_local, 271.6 M rows, 3,928 parts, post-cutover):Condition: true)And an actual delete of 12 ids rewrote 3,928 parts per replica / 5.40 TiB. It also returned HTTP 500 to the caller —
java.net.ConnectException: Read timed outatTracesResource.deleteTraces:401— while the mutation completed server-side. That symptom is not fixed here; it is a separate issue, but this change removes its main cause.Why a set and not an
id_atrangeAn
id_atrange is not a substitute. On a batch spanning 1996 and 2200, a range still selected 2,644 of 3,928 parts, because it covers every week in between; the exact set selected 4. Scattered batches are the normal case for a delete-by-ids API.The correctness guarantee is preserved deliberately
The original javadoc says the omission was intentional: "No
id_at/time predicate on purpose, so it still deletes rows whoseid_atis untrustworthy." That property is kept. The predicate is emitted only when every id in the batch is a UUIDv7. All-or-nothing per batch, so the SQL is either fully pruned or byte-identical to the previous form — never partially bounded. Deriving a partition from a non-v7 id would read whatever bits sit in the timestamp field, and a wrong partition is a silently skipped delete, which is worse than a slow one.Far-future ids are deliberately supported, not excluded: their
id_atis bogus but self-consistent, so they live in the far-future partition this computes. Verified on prod-test — 0 mismatches betweentoMonday(id_at)andtoMonday(UUIDv7ToDateTime(id))across all 11.23 M far-future rows (4.1% of the table).DELETE_FOR_RETENTIONalready carries equivalent bounds for the same reason (OPIK-6900). This brings the by-ids path in line, plus the v7 guard its arbitrary id set requires.Change checklist
Issues
Documentation
N/A for user-facing docs. The change is internal to the delete path and is documented where it has to be maintained: the javadoc on
DELETE_BY_PROJECT_ID_TRACE_ID_PAIRSexplains why the predicate is conditional, and the javadoc onweeklyPartitionsOfrecords that it mirrors the table's partition expression exactly, so a change to that expression has a stated place to be reflected.AI-WATERMARK
AI-WATERMARK: yes
EXPLAIN indexes=1and a real delete; wrote the change and its tests.id_atrange I first suggested — that is what produced the 4-parts-vs-2,644 result, and it is the design in this PR. Reviewer sign-off still required.Testing
TraceDAOPartitionPruningTest— 8 cases. The expected partition values are not hand-computed: each is what ClickHouse itself returned fortoYYYYMMDD(toDate32(id_at) - toIntervalDay(toDayOfWeek(id_at, 1)))on prod-test for that id, so if the partition expression ever changes these assertions are what fail.20260817; far-future id (2200-01-01) →21991230; 1996 id →19960205{20260817, 19960205}The partition arithmetic was also executed standalone against those three ClickHouse-derived values before being committed, and all three matched.
🤖 Generated with Claude Code