-
[OPIK-6177] [BE] fix: dedupe by stable dataset_item_id in experiment comparison (#6507)
frostbyte_neo 发布于
2026-05-07 14:58:41 +00:00 - [OPIK-6177] [BE] fix: dedupe by stable dataset_item_id in experiment comparison
When comparing experiments whose runs span multiple dataset versions of
the same item, the comparison endpoint returned one row per
(dataset_item_id, dataset_version_id) instead of one row per stable
dataset_item_id. The FE then rendered each row separately, showing
"skipped" for the experiments not present in that row's
run_summaries_by_experiment.Collapse dataset_items_resolved and dataset_items_aggr_resolved CTEs to
one row per stable dataset_item_id (latest version) via LIMIT 1 BY
dataset_item_id, and drop the now-redundant
di.dataset_version_id = ei.resolved_dataset_version_id constraint from
the LEFT JOINs so each ei row matches the canonical di row regardless of
which version the experiment was linked to.Implements OPIK-6177: Experiment Comparison is duplicating rows in
direct comparison of two test suites- fix(dataset-items): drop version-equality on top_dataset_items push-top join
Mirror the same change to the push-top-limit branch's
dataset_items_aggr_resolved
join. Without it, when comparing aggregated experiments linked to different
dataset versions of the same item, items whose linked version is older than the
canonical (latest) one indataset_items_aggr_resolvedproduced NULLdi_t.*
in the JOIN, makingany(di_t.*)sort expressions unstable and pagination
non-deterministic.Also removes the now-unused INNER JOIN to
experiment_aggregated_scope_ids—
its only purpose was to provideeas_t.resolved_dataset_version_idfor the
predicate.Caught by reviewer.
- refactor(dataset-items): adopt Option 5 — resolve ei.dataset_item_id to stable id upstream
Replaces this PR's earlier OPIK-6177 approach (which regressed OPIK-4518's
legacy row_id case per #5852) with andrescrz's Option 5 design from the
review. The resolution CTE pattern produces astable_dataset_item_idfor
every experiment_items row regardless of whether ei.dataset_item_id holds
the stable id (modern writes, post-OPIK-4518 BE cutover) or a per-version
dataset_item_versions.id (legacy writes pre-cutover).Resolution shape: inline LEFT JOIN to dataset_item_versions FINAL keyed on
(workspace_id, id) — the per-version row PK. For modern rows the JOIN
misses andif(notEmpty(lookup_div.dataset_item_id), …, ei.dataset_item_id)
falls back to ei.dataset_item_id (already the stable id). For legacy rows
the JOIN matches and projects div.dataset_item_id (the stable id).A standalone
ei_id_lookupCTE was tried first but caused ClickHouse's
analyzer to drop left rows when the CTE materialized empty (deletion-cascade
scenarios where dataset_item_versions has no rows for the dataset). Direct
table reference avoids that.Downstream JOIN simplifies from
(di.id = ei.dataset_item_id OR di.row_id = ei.dataset_item_id) AND di.dataset_version_id = ei.resolved_dataset_version_id
to the single condition
di.id = ei.stable_dataset_item_id,
removing the OR-JOIN structural smell from 5 sites in the comparison row +
count + push-top + stats queries plus 2 sites in ExperimentAggregatesDAO,
and dropping #5852's per-experiment version-equality predicate (it is no
longer needed once resolution happens upstream).GROUP BY in both UNION branches now keys on stable_dataset_item_id; the
existingLIMIT 1 BY dataset_item_idin dataset_items_*resolved gives one
canonical (latest-version)dirow per stable id, so per-versiondi.*
columns in GROUP BY are functionally determined by stable_dataset_item_id
and don't split rows.buildTopItemsSorting/getTopSortExpressionupdated to project the
resolved expression for sorting fields that reference the experiment-side
key.Implements Option 5 as agreed with @andrescrz in the PR review thread.
Fixes the OPIK-6177 duplicate-row symptom AND restores OPIK-4518's legacy
row_id rendering in a single, unified shape.All 549 comparison-endpoint + stats + ExperimentAggregatesIntegration tests
pass (1 pre-existing flake unrelated to these changes).- chore(backend): remove debug log accidentally left in DAO
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- fix(backend): dedupe across UNION branches in compare query
When one experiment has rows in experiment_aggregates and another does
not (transient post-experiment window before aggregation catches up),
the compare query unioned both branches without an outer dedup, so the
same stable_dataset_item_id surfaced twice — once with experiment A,
once with experiment B — instead of merging into a single row whose
experiment_items_array carries both.Adds an outer GROUP BY u.id over the UNION ALL with:
- groupArrayArray(experiment_items_array) to flatten both branches'
per-experiment item arrays into one merged array. - argMax(col, last_updated_at) for dataset-derived scalars so the row
shows the newer-version's data when branches resolved different
dataset_version_ids — matching single-branch latest-wins semantics. - max(last_updated_at) for the timestamp itself.
Inner branches stay unchanged. Count, stats, and push-top variants are
unaffected (count already wraps with COUNT(DISTINCT); stats union by
ei.id which is disjoint between branches; push-top is gated by !hasRaw).Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- test(backend): add cross-branch dedup regression for compare endpoint
Verifies that when one experiment is in experiment_aggregates and
another is not (mixed hasAggregated=true && hasRaw=true), the compare
endpoint returns one merged row per stable_dataset_item_id with both
experiments' items in experiment_items_array — not 2× rows split by
branch as it did before the outer GROUP BY fix.Confirmed sensitive: fails with "Expected size: 5 but was: 10" when the
outer GROUP BY is reverted; passes with the fix.Also wires Injector into setUpAll to inject ExperimentAggregatesService
so populateAggregations(...) can be invoked from tests in this file.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- style(test): use imported ArrayList/HashSet instead of FQN in new test
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- feat(backend): backfill experiment_item_aggregates.dataset_item_id to stable id
OPIK-6177 — adds Liquibase migration 000084 that rewrites legacy rows in
experiment_item_aggregates whose dataset_item_id is a per-version row_id
(pre-OPIK-4518 BE cutover). After this migration, eia.dataset_item_id IS
the stable dataset_item_id (matches dataset_item_versions.dataset_item_id),
which lets the compare query reference the column directly without a
per-query lookup_div LEFT JOIN — restoring skip-index pushdown and
fixing the ~11x regression thiagohora measured.Mechanism: INSERT INTO experiment_item_aggregates SELECT ... INNER JOIN
dataset_item_versions FINAL ... WHERE the lookup actually changes the
value. ReplacingMergeTree dedupes on (workspace_id, experiment_id, id)
PK with last_updated_at (set to now64(9) here) as version column, so
the rewritten rows supersede the legacy ones on next merge. Filter
condition limits the rewrite to legacy rows only — modern writes (where
eia.dataset_item_id is already the stable id) are untouched.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- feat(backend): resolve stable dataset_item_id at aggregation time
OPIK-6177 — modifies GET_EXPERIMENT_ITEMS to LEFT JOIN dataset_item_versions
and project the resolved stable dataset_item_id when reading raw experiment_items
into the aggregation pipeline. New aggregations write the stable id directly to
experiment_item_aggregates (going forward); the 000084 migration rewrites legacy
rows that pre-date this change.For modern post-OPIK-4518 writes, ei.dataset_item_id is already the stable id,
the LEFT JOIN typically misses (or matches a v1 DIV row whose dataset_item_id
== id), and the if(notEmpty(...)) fallback returns the same value — no behavior
change. For legacy writes where ei.dataset_item_id is a per-version row id,
lookup_div finds the DIV row and projects its (stable) dataset_item_id —
fixes the OPIK-4518 backwards-compat case at write time.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- perf(backend): drop read-time lookup_div from aggregated branch + swap argMax tiebreaker
OPIK-6177 — now that experiment_item_aggregates.dataset_item_id is the stable id
(via the 000084 migration + write-time resolution), the compare query references
eia.dataset_item_id directly, dropping the per-query LEFT JOIN dataset_item_versions
and if(notEmpty(...)) wraps. This restores the minmax skip-index pushdown that
thiagohora measured as ~11x faster on the OPIK-6311 push-top path (~10.88s → ~0.99s
for filtered queries; full skip-index pruning of experiment_item_aggregates from
~341 of 342 granules read down to ~2 of 342).Sites simplified (all aggregated-branch):
- top_dataset_items (push-top, !push_top_needs_div) — drops lookup_div join + filter wrap
- top_dataset_items (push_top_needs_div) — same
- item_agg_count CTE (count query) — same
- item_agg inner SELECT in row query — same
- buildTopItemsSorting / getTopSortExpression helpers — direct eia_t.dataset_item_id
Cross-branch outer GROUP BY also fixed for MEDIUM #5: swap argMax key from
last_updated_at to dataset_version_id so cross-branch and single-branch agree on
which version's metadata wins (single-branch dedup is dataset_version_id DESC,
LIMIT 1 BY dataset_item_id — the new outer argMax matches that semantic).
Adds di.dataset_version_id projection through both inner branches.Raw branch's lookup_div is unchanged — it reads from experiment_items (source
table) which is not migrated; raw-branch volume is small (only experiments not
yet aggregated), and the OPIK-4518 backwards-compat for legacy rows there
remains valuable.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- test(backend): add EIA-filter cross-version regression test for OPIK-6177
Adds multiVersionExperimentsEiaFilterConsistentBeforeAndAfterAggregates,
mirroring multiVersionExperimentsFilterConsistentBeforeAndAfterAggregates
but exercising an experiment_item_aggregates-side filter (duration > 0)
rather than a dataset_item filter. Both filters activate push_top_limit's
top_dataset_items CTE differently:- DI-side filter renders dataset_items_filtered_ids CTE (existing test)
- EIA-side filter renders the experiment_item_filters clause directly
against eia.duration (new test, post-OPIK-6177 stable-id migration
preserves skip-index pushdown on this path)
Asserts before-aggregation result == after-aggregation result for the
same two experiments at distinct dataset versions, locking in the
correctness contract regardless of which branch (raw vs aggregated)
serves the request.Addresses @baz-reviewer's MEDIUM #6 from PR #6507 review.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- style(backend): add trailing empty line to OPIK-6177 migration per migrations.md convention
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- perf(backend): drop lookup_div from FromAggregates dev/test queries
OPIK-6177 — extends the same simplification applied to the production
DatasetItemVersionDAO compare query (commit23da40bdde) to the
ExperimentAggregatesDAO dev/test parity queries
(SELECT_DATASET_ITEM_VERSIONS_WITH_EXPERIMENT_ITEMS_COUNT and
SELECT_DATASET_ITEM_VERSIONS_WITH_EXPERIMENT_ITEMS).These two queries are reached via getDatasetItemsWithExperimentItemsFromAggregates
and countDatasetItemsWithExperimentItemsFromAggregates, used as a parity
harness in ExperimentAggregatesIntegrationTest to validate that the
"from aggregates" path returns the same results as the original DAO.
Not in the production hot path, but kept for consistency with the prod
query post-migration.Post-migration eia.dataset_item_id IS the stable id, so direct column
references replace the lookup_div LEFT JOIN + if(notEmpty(...)) wrap
in 3 sites (count clause + count JOIN-on + find JOIN-on). Skip indexes
applicable in dev/test runs too. 18 parity tests still pass; 138/138
EAI integration tests green.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- revert(backend): drop OPIK-6177 backfill migration approach
Reverts the migration-based path per @andrescrz's CHANGES_REQUESTED on PR
#6507 ("Let's avoid introducing a data migration here"), aligned with
@thiagohora's follow-up ("ship the change in the aggregation population
and discuss about the backfill separately").This squashes 4 reverts into one to keep history readable:
f2bd5d6da4(Liquibase migration 000084)c1f4f9b011(trailing newline in migration file)23da40bdde(read-path lookup_div drop + argMax swap — read-path drop
depended on the migration; argMax swap will be re-applied on top of
Alt 7)4de440d5c9(dev/test query lookup_div cleanup — same dependency)
Keeps:
08b184b619(write-time stable_dataset_item_id resolution in
GET_EXPERIMENT_ITEMS) — no behavior change to existing rows; cleans
the data going forward, useful when revisiting backfill in a
follow-up.febcee05c9(cross-branch outer GROUP BY for Bug 2)abed2c67c2/af864c5ae4(regression tests)
State after this revert is pre-migration: lookup_div LEFT JOIN +
if(notEmpty(...)) wraps remain in the aggregated-branch read path, which
is what triggered @thiagohora's perf concern on the OPIK-6311 path. Next
commit replaces that pattern with Alternative 7 (slim
eligible_dataset_item_lookup CTE + plain IN/equi-JOIN), preserving
OPIK-4518 backwards-compat AND skip-index pushdown without a backfill
migration.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- fix(backend): Alt 7 — slim id-resolution CTE for read-time stable id lookup
OPIK-6177 — addresses @andrescrz's CHANGES_REQUESTED ("Let's avoid introducing
a data migration here") on PR #6507. Replaces the deferred-but-rejected
backfill migration with a read-time resolution that satisfies all three
constraints:(a) OPIK-4518 backwards-compat for legacy experiment_items rows that store
per-version row_ids
(b) OPIK-6311 perf path (no if(notEmpty(lookup_div...)) wraps that defeat
the EIA-side idx_experiment_item_aggregates_dataset_item_id minmax
skip index)
(c) No data migrationShape: a slim eligible_dataset_item_lookup CTE scoped to current experiments'
dataset versions emits both per-version row_ids AND stable_dataset_item_ids
via arrayJoin([div.id, div.dataset_item_id]). Used as a skip-index-friendly
IN list when DI filters are active —eia.dataset_item_id IN (SELECT lookup_id FROM eligible_dataset_item_lookup)— same pattern OPIK-6311 measured at
13× speedup.Notably the CTE is NOT used as a JOIN target. A CTE-based LEFT JOIN drops
rows on deletion-cascade in ClickHouse (known analyzer behavior, also
documented inline on experiment_items_scope from a prior incident). The
resolution itself uses LEFT JOIN against the direct dataset_item_versions
table — same shape as Option 5 / experiment_items_scope's raw-branch JOIN.Sites updated:
DatasetItemVersionDAO.java (compare query, aggregated branch)
- Count query item_agg_count: added eligible_dataset_item_lookup CTE,
direct DIV LEFT JOIN, IN-against-CTE WHERE filter when dataset_item_filters
is active. - Row query item_agg inner SELECT: same pattern.
- top_dataset_items × 2: reverted to main's raw eia.dataset_item_id shape
(resolution happens in outer row query). - Sorting helpers (buildTopItemsSorting, getTopSortExpression): reverted
to eia_t.dataset_item_id. - Outer cross-branch GROUP BY argMax tiebreaker: re-applied
last_updated_at → dataset_version_id swap (re-applies @baz-reviewer's
MEDIUM #5; was reverted along with the migration approach).
ExperimentAggregatesDAO.java (FromAggregates dev/test queries)
- Both COUNT and FIND queries: same pattern (direct DIV LEFT JOIN +
if(notEmpty(...))) for parity with the production query. - GET_EXPERIMENT_ITEMS write-path resolution kept as best-effort hygiene
(cleans new EIA rows going forward without a backfill migration).
All 251 integration tests pass: 138 ExperimentAggregatesIntegrationTest +
113 DatasetsResourceTest$FindDatasetItemsWithExperimentItems* +
GetDatasetExperimentItemsStats. Includes the deletion-cascade test that
caught the CTE-LEFT-JOIN issue in earlier iterations.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- style(backend): move OPIK-6177 SQL inline comments into Java
Per @thiagohora's review: SQL templates should not carry rationale
comments inline; the prose belongs as Java // comments above the
relevant SQL constants where it doesn't clutter the rendered query.Hoists 6 multi-line
-- ...blocks out of the SQL templates and
re-attaches their content as Java//comment blocks above the
correspondingprivate static final String <NAME>declarations.
Same context preserved (stable-id resolution, deletion-cascade
ClickHouse-analyzer note, eligible_dataset_item_lookup CTE rationale,
outer cross-branch GROUP BY argMax tiebreaker semantics) — just at
the Java level.No SQL change. 138/138 ExperimentAggregatesIntegrationTest +
64/64 compare-experiments tests pass.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- perf(backend): gate eligible_dataset_item_lookup CTE on !push_top_limit
Per @thiagohora's EXPLAIN benchmark on the v2 shape (review 4229616845):
the eligible_dataset_item_lookup CTE was reading ~33% of dataset_item_versions
on the push_top_limit path because its WHERE clause filters by
dataset_version_id, a non-leading PK column that doesn't engage the
dataset_item_id-targeted skip indexes. Resulted in a 2.62× regression vs
main on a representative experiment (page-25 sorted by id DESC, DI-side
filter, 5 rounds: main 1.005s vs v2 2.634s).When push_top_limit is on, the inner SELECT already has
eia.dataset_item_id IN (SELECT dataset_item_id FROM top_dataset_items)
which constrains EIA to ~25 paged items. The lookup_div FINAL LEFT JOIN
then translates whichever id form was stored back to stable_dataset_item_id
via the if(notEmpty(...)) fallback — single-granule lookup because the IN
pre-prunes EIA. The CTE-based IN predicate is purely defensive on this
path and adds no correctness benefit.@thiagohora's bench confirmed dropping the CTE on the push-top path is
byte-identical (MD5-match across 25 result rows) and recovers ~99% of
the optimization (1.088s — essentially main's 1.005s).Implementation: in SELECT_DATASET_ITEM_VERSIONS_WITH_EXPERIMENT_ITEMS,
wrap the eligible_dataset_item_lookup CTE definition in <if(!push_top_limit)>
and gate the IN-predicateeia.dataset_item_id IN (SELECT lookup_id ...)
the same way. The count query (SELECT_DATASET_ITEM_VERSIONS_WITH_EXPERIMENT_ITEMS_COUNT)
is unchanged — it doesn't go through top_dataset_items so the eligibility
filter still serves a purpose there.Tests: deletion-cascade canary
(experimentItemsForDeletedDatasetItemConsistentBeforeAndAfterAggregates) +
113 compare-experiments tests pass. Full EAI suite running in background.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- perf(backend): collapse eligible_dataset_item_lookup into inline arrayJoin IN
Address PR #6507 review 4229992011: count-query path was 2.85x slower
than main (1.517s -> 4.327s) because eligible_dataset_item_lookup did
an unindexed dataset_item_versions FINAL scan. Drop the CTE and use
the same inline arrayJoin([id, row_id]) FROM dataset_items_agg_resolved
pattern that main already uses, so the IN list can leverage the EIA
skip index. Same fix applied to the row query's !push_top_limit
branch; push_top_limit branch retained its existing outer-id IN check
(top_dataset_items already filters there).Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- fix(backend): cover all-versions in count IN-set via lookup_for_count CTE
Address PR #6507 baz-reviewer finding (comment 3190434359) and
thiagohora's confirmation (comment 3191033224): after collapsing
eligible_dataset_item_lookup, count's arrayJoin([id, row_id]) FROM
dataset_items_agg_resolved (LIMIT 1 BY dataset_item_id) only emits
the LATEST version's row_id per item, so legacy EIA rows referencing
an OLDER version's row_id (pre-OPIK-6177 + post-version-bump) fail
the IN-check and undercount. Push-top row branch is unaffected because
its lookup_div FINAL handles all versions.Add a lookup_for_count CTE that narrows by <dataset_item_filters>
first then INNER JOINs dataset_item_versions FINAL, emitting both
div.id (every version's row_id) and the stable id. Bloom skip-index
on dataset_item_id helps because the inner join is dataset_item_id-
narrowed (unlike the previously-removed eligible CTE which was
version-only-narrowed and couldn't prune). Bench (per thiagohora):
count 1.964s vs 1.827s (+0.14s) and full cross-version correctness.Same shape applied to row query !push_top_limit branch (CTE name
shared because the queries are separate StringTemplate constants).Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- docs(backend): update OPIK-6177 comments to reference lookup_for_count
The eligible_dataset_item_lookup CTE was replaced by lookup_for_count
in1c34a84061. Update the two stale Javadoc/inline comment blocks in
DatasetItemVersionDAO to reference the new CTE name and capture the
narrow-by-filter / arrayJoin shape that covers all-version row_ids.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- perf(backend): drop FINAL on dataset_item_versions reads + clean up CTEs
Address PR #6507 andrescrz feedback (comments 3194973428, 3195000400,
3195054140, 3195055228):- Remove dead eligible_dataset_item_lookup CTE definitions in
ExperimentAggregatesDAO (the count + row parity-harness queries)
that were never referenced in their SQL — leftover from the Alt 7
shape that didn't end up wiring the IN predicate in this file. - Promote OPIK-6177 // inline comment blocks above SQL constants
into proper Javadoc in both files (per andrescrz's nit). - Drop FINAL on every dataset_item_versions read in both files. The
table is a ReplicatedReplacingMergeTree where every writer generates
a fresh per-row id (BATCH_INSERT_ITEMS, EDIT_ITEM_VIA_SELECT_INSERT,
COPY_VERSION_ITEMS) and the one-shot COPY_ITEMS_FROM_LEGACY is
double-guarded by DatasetVersioningMigrationService.ensureDataset
Migrated, so no write path re-INSERTs the same (workspace_id,
dataset_id, dataset_version_id, id) tuple. Pre-merge duplicates
can't arise; FINAL was theoretical defense. Documented in the
SELECT_DATASET_ITEM_VERSIONS_WITH_EXPERIMENT_ITEMS Javadoc with a
note for future maintainers to restore FINAL if any non-key column
becomes mutable via re-INSERT.
Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
- fix(backend): restore FINAL on dataset_item_versions reads
Revert the FINAL drop from
5a56e2fc5a. The immutability assumption
documented there was wrong: PUT /datasets/items is a real upsert flow
that re-INSERTs the same (workspace_id, dataset_id, dataset_version_id,
id) tuple when a client PUTs the same item ids twice with changed data,
description, tags, evaluators, or execution_policy. Pre-merge
duplicates are routine on this table; FINAL is required so reads see
only the latest row per PK.CI surfaced this via DatasetsResourceTest$GetDatasetItemsByDatasetId.
getDatasetItemsByDatasetId__whenItemsWereUpdated__thenReturnCorrectItemsCount
which PUTs items twice with changed data and asserts that
SELECT_COLUMNS_BY_VERSION returns only the latest version's columns —
without FINAL the query saw both pre-merge rows.Restored FINAL only where reads don't already dedupe via
LIMIT 1 BY dataset_item_id ORDER BY (..., dvid) DESC, last_updated_at
DESC (those subqueries pick the latest row themselves so FINAL is
redundant on the inner scan). Updated the
SELECT_DATASET_ITEM_VERSIONS_WITH_EXPERIMENT_ITEMS Javadoc to capture
that FINAL is load-bearing for this table given the upsert contract.Keeps the dead eligible_dataset_item_lookup CTE removal and Javadoc
promotions from5a56e2fc5a— those remain correct.Co-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com
Co-authored-by: Claude Opus 4.7 (1M context) noreply@anthropic.com
下载附件