[OPIK-7707] [QA] Proposed e2e specs: dataset version counters on the delete path - #7965
CometActions wants to merge 5 commits into
Conversation
Replace the read-modify-write count update on the dataset item insert path with a single atomic SQL increment, and drop the version re-read that immediately followed it. MySQL round-trips inside the per-dataset lock drop from three (find, update, re-read) to one. The returned DatasetVersion is read by no consumer -- the REST endpoint returns 204 with the blocked result unassigned, and saveBatch maps it to the item count -- so insertItemsIntoVersion now completes empty instead of fabricating a partially populated object. The delete path is converted to the same atomic increment for consistency. Unlike the insert path it saves no round-trip: both callers already fetch the version for their own logging. What it gains is that the counter arithmetic no longer depends on withDatasetVersionLock for mutual exclusion. Implements OPIK-7707: Make the version count update atomic and drop the post-insert version re-fetch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
updateVersionCountsForDelete ignored the affected-row count from incrementCounts, so insert and delete disagreed about how a missing or cross-workspace version is handled. Both callers currently fetch the version first via getVersionById, which already throws, so this is not reachable today -- but the asymmetry is a trap for later work that narrows the lock or drops that pre-fetch. Also drop the helper's log.info: the delete callers already emit a summary with deletedCount, versionId and the resulting total two frames up. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
updateVersionCountsForInsert and updateVersionCountsForDelete had become identical apart from which deltas they pass -- same transaction, same DAO call, same zero-row NotFoundException guard -- so any change to the atomic update or the error handling had to be mirrored in both. Both now delegate to a shared updateVersionCounts(...) taking signed deltas. The named wrappers stay so the call sites still read as insert/delete intent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The counter columns are `INT DEFAULT 0` (migration 000036), which is nullable -- DEFAULT only applies when the column is omitted on INSERT, and DatasetVersion maps all four as boxed Integer. A bare `col + :delta` leaves NULL as NULL, which then unboxes to an NPE on read. The absolute update this replaced happened to repair a NULL by overwriting it, so the increment has to COALESCE explicitly or it is a regression in NULL tolerance. Adds a test that nulls the counters directly and asserts the increment treats them as zero. Verified it fails without the COALESCE. Ported from the duplicate work on #7928, which is being closed in favour of this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OPIK-7707 (#7705) put both dataset-version write paths behind one signed-delta UPDATE. The insert side is observable through existing specs only as rendered rows; the delete side is not covered at all — `dataset-items.spec.ts` bulk-deletes but counts table rows and SDK items, never a counter, so an items_total that kept climbing after a delete would leave the suite green. Adds `datasets/dataset-version-delete-counters.spec.ts`: two @t2-cuj specs driving the ungrouped write path (no batch_group_id, so the latest version is mutated in place) and asserting, after every operation, the exact counter quadruple, that the version was mutated and not replaced, and that items_total equals the item ids the dataset actually holds — then reading the same figures back off the Version history tab. Also adds the backend-client reads those need (getDatasetVersions, listDatasetItemIds, ungrouped upsert/delete-by-id) and the DatasetItemsPage locators for the Version history tab, and flips datasets.version-history-view to covered in the taxonomy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
tests
📋 PR Linter Failed❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the ❌ Missing Section. The description is missing the |
⏱️ pre-commit per-hook timingNo linted files changed — nothing to run. ⏭️ 41 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| async getDatasetVersions(datasetId: string): Promise<DatasetVersionRef[]> { | ||
| const page = await opik.api.datasets.listDatasetVersions(datasetId, { size: 100 }); | ||
| const content = page.content ?? []; |
There was a problem hiding this comment.
Version history silently truncates
getDatasetVersions returns only the first 100 results as a complete-looking DatasetVersionRef[], so datasets with more versions are silently truncated and callers can make incorrect version-count conclusions — should we fetch all pages or expose an explicit pagination contract?
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
`tests_end_to_end/e2e/core/backend/client.ts` around lines 257-269, refactor
`getDatasetVersions` so it does not silently truncate the paginated version history at
100 results. Fetch successive pages using the API’s pagination parameters until the
final page is reached, accumulate all results, and then map them to `DatasetVersionRef`;
add or update coverage for datasets containing more than 100 versions.
| id: String(v.id ?? ''), | ||
| versionName: String(v.versionName ?? ''), | ||
| itemsTotal: Number(v.itemsTotal ?? 0), | ||
| itemsAdded: Number(v.itemsAdded ?? 0), | ||
| itemsModified: Number(v.itemsModified ?? 0), | ||
| itemsDeleted: Number(v.itemsDeleted ?? 0), | ||
| isLatest: Boolean(v.isLatest), |
There was a problem hiding this comment.
Missing counters silently pass as zero
The adapter’s ?? 0 defaults let omitted or null itemsModified/itemsDeleted values satisfy the seeded-version assertion without verifying the API response. Because parseOrThrow uses skipValidation: true, Boolean(v.isLatest) also turns malformed values such as "false" into true — should we validate required finite numeric/boolean fields and throw with their names instead of defaulting or coercing them?
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
`tests_end_to_end/e2e/core/backend/client.ts` around lines 261-267, update
`getDatasetVersions` so it does not default missing/null counters to zero or coerce
`isLatest` with `Boolean(...)`. Explicitly validate that each required counter is a
finite number and that `isLatest` is actually a boolean, throwing an error that names
the invalid field when validation fails; preserve valid zero and false values.
| async deleteDatasetItemsByIds(itemIds: string[]): Promise<number> { | ||
| const { rawResponse } = await opik.api.datasets | ||
| .deleteDatasetItems({ itemIds }) | ||
| .withRawResponse(); |
There was a problem hiding this comment.
Repeat-delete test bypasses idempotent branch
The item-ID-only helper sends itemIds, so once the first ungrouped ALTER ... DELETE is visible, resolveDatasetIdsFromItemIds makes getDatasetIdOrResolveItemDatasetId return Mono.empty() before deleteItemsFromExistingVersion/removeItemsFromVersion, yet the resource returns HTTP 204 and the assertion skips the documented existingCount == 0 no-counter path; if rows remain visible, it can instead delete and count them again, making the test timing-dependent. Since DatasetItemsDeleteValidator rejects adding dataset_id alongside item_ids, should we add a backend-supported scoped-ID retry contract or exercise the no-op branch through a supported path?
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
tests_end_to_end/e2e/core/backend/client.ts around lines 323-326, fix
deleteDatasetItemsByIds and its callers so a repeated ungrouped delete reliably reaches
the documented existingCount == 0 no-op path instead of being skipped by dataset-ID
resolution or depending on ClickHouse visibility timing. Do not add datasetId alongside
itemIds, because DatasetItemsDeleteValidator rejects that request; implement a
backend-supported scoped-ID retry contract if available, otherwise change the test to
use a supported deletion path that reaches and asserts the no-counter branch.
| versionHistoryRow(versionName: string): Locator { | ||
| return this.versionsTableBody | ||
| .locator('tr[data-row-id]') | ||
| .filter({ has: this.page.getByRole('cell', { name: versionName, exact: true }) }); | ||
| } |
There was a problem hiding this comment.
Version-history POM failures lack named steps
versionHistoryRow, versionItemCount, versionChangeSummary, versionChangeTags, and versionChangeTag return locators without named test.step(...) boundaries, so the outer spec step doesn't provide the required per-method POM boundary — should we wrap each method in a descriptive step and return its callback result, as .agents/skills/writing-e2e-tests/SKILL.md and .agents/skills/writing-e2e-tests/conventions.md require?
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
`tests_end_to_end/e2e/pom/dataset-items.page.ts` around lines 80-124, update
`versionHistoryRow`, `versionItemCount`, `versionChangeSummary`, `versionChangeTags`,
and `versionChangeTag` to comply with the E2E POM convention requiring every method to
use a named `test.step(...)` boundary. Wrap each method body in a descriptive step and
return the locator from the step callback, preserving the existing locator behavior and
method signatures.
| return { versionId: version.id, itemIds }; | ||
| }); | ||
|
|
||
| let held = seeded.itemIds; |
There was a problem hiding this comment.
Obscures stored item ID invariant
held is ambiguous for the full set of stored dataset item IDs passed as expected.itemIds — should we rename it to itemIds (or currentItemIds) and update all uses, including the later held.slice(...) calls?
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
`tests_end_to_end/e2e/tests/datasets/dataset-version-delete-counters.spec.ts` around
lines 202-263, rename the `held` variable in the interleaved inserts, upserts, and
deletes test to `currentItemIds` (or `itemIds`) so it clearly represents the complete
set of currently stored dataset item IDs. Update every assignment, filter, spread,
`expected.itemIds` reference, and later `held.slice(...)` call consistently.
624b8dc to
bf83f88
Compare
| int expectedIncrements = writers * incrementsPerWriter; | ||
| var version = getLatestVersion(datasetId); | ||
| assertThat(version.itemsTotal()).isEqualTo(1 + expectedIncrements); | ||
| assertThat(version.itemsAdded()).isEqualTo(1 + expectedIncrements); | ||
| assertThat(version.itemsModified()).isEqualTo(2 * expectedIncrements); |
There was a problem hiding this comment.
Concurrent test misses delete-counter corruption
The concurrent increment test doesn't check itemsDeleted, so a regression that changes this counter would still pass — should we assert version.itemsDeleted() remains zero?
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/api/resources/v1/priv/DatasetVersionResourceTest.java`
around lines 5321-5325, update
`incrementCounts__whenConcurrentWritersBypassTheLock__thenCountersAreExact` to verify
the complete counter invariant. Add an explicit assertion that `version.itemsDeleted()`
remains zero, since the test passes a zero deleted delta and should detect regressions
that modify or overwrite that counter.
| int incrementCounts(@Bind("version_id") UUID versionId, | ||
| @Bind("items_total_delta") int itemsTotalDelta, | ||
| @Bind("items_added_delta") int itemsAddedDelta, | ||
| @Bind("items_modified_delta") int itemsModifiedDelta, | ||
| @Bind("items_deleted_delta") int itemsDeletedDelta, |
There was a problem hiding this comment.
Misleading name hides decrement behavior
incrementCounts applies signed deltas, including the negative itemsTotalDelta from updateVersionCountsForDelete, so its name can mislead callers about deletion support and argument signs — should we rename it to applyCountDeltas and update the Javadoc/callers?
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/DatasetVersionDAO.java around
lines 557-561, rename `incrementCounts` to an explicit signed-delta name such as
`applyCountDeltas`, since deletion callers pass negative values and the SQL adds signed
deltas. Update the method’s Javadoc and every caller, including
`updateVersionCountsForDelete`, so the API clearly communicates that positive and
negative counter adjustments are supported.
| log.info("Inserting '{}' items into existing version '{}'", batch.items().size(), latestVersionId); | ||
|
|
||
| return insertItemsIntoVersion(batch, datasetId, latestVersionId, workspaceId, userName); | ||
| return insertItemsIntoVersion(batch, datasetId, latestVersionId, workspaceId, userName).then(Mono.empty()); |
There was a problem hiding this comment.
Versioned append loses promised result
The existing-version paths in save(DatasetItemBatch) call insertItemsIntoVersion(...).then(Mono.empty()), so callers that block() or subscribe receive no DatasetVersion after a successful append—should we emit the mutated version from both branches?
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/DatasetItemService.java` around
lines 1665 and 2359-2360, fix `mutateLatestVersionWithInsert` and
`handleGroupedInsertion` so successful updates emit the resulting `DatasetVersion`
instead of completing with `Mono.empty()`. Refactor `insertItemsIntoVersion` or append a
properly scheduled fetch after the insert and atomic count update, ensuring both
existing-version paths satisfy the `save(DatasetItemBatch)` return contract while
preserving the new-version behavior.
| * Addressed by the table's own `data-cell-id` (`<rowId>_<columnId>`) rather | ||
| * than a positional nth(): the version table has no per-cell testid, and | ||
| * column order is user-configurable, so position is not stable. | ||
| */ | ||
| versionItemCount(versionName: string): Locator { | ||
| return this.versionHistoryRow(versionName).locator('[data-cell-id$="_items_total"]'); |
There was a problem hiding this comment.
Brittle CSS locators bypass required test IDs
The POM relies on structural [data-cell-id$=...] CSS locators, so column-order changes make the version-history tests brittle — should we add scoped descriptive kebab-case data-testid attributes through the existing change_summary cell components and a page-local items_total renderer, then use getByTestId as .agents/skills/playwright-pom-discovery/SKILL.md and .agents/skills/writing-e2e-tests/conventions.md require?
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
`tests_end_to_end/e2e/pom/dataset-items.page.ts` around lines 90-95, update the
version-history cell locator logic to stop relying on structural `[data-cell-id$=...]`
CSS selectors. Add descriptive, scoped kebab-case `data-testid` attributes to the
frontend renderers for the `items_total` and `change_summary` cells, then have
`versionItemCount` and `versionChangeSummary` locate those cells with `getByTestId`
while preserving row scoping; do not modify the generic DataTable.
3 participants
Where these came from
Exploratory testing of #7705 —
[OPIK-7707] [BE] perf: make dataset version count updates atomic, run by hand against that PR's own deployed environment (pr-7705.dev.comet.com, serving2.2.14-7705-merge-3043; fresh OSS install, workspacedefault). Every flow below was reached and verified manually there before any spec was written.This PR targets
danield/OPIK-7707-make-the-version-count-update-atomic, notmain— it is branched from that PR's head,7ad36cf35490e454fb93810311c374361354a67f. The specs assert the counter arithmetic the way #7705 implements it (one signed-deltaUPDATEshared by the insert and delete paths), so they belong on top of the change they test rather than ahead of it. Merge #7705 first; this then rides in with it.What was uncovered
updateVersionCountsForInsertandupdateVersionCountsForDeletenow funnel throughupdateVersionCounts(versionId, workspace, totalDelta, addedDelta, modifiedDelta, deletedDelta, user)— six positional ints, which is where a sign or argument slip would hide. Nothing in the suite reads a version counter on the delete side:dataset-items.spec.tsbulk-deletes but counts rendered table rows and SDK items, so anitems_totalthat kept climbing after a delete would leave every existing spec green, while the Version history tab showed users the wrong Item count.datasets.version-history-viewwascovered: falsein the taxonomy at this branch point.The specs
Both live in
tests_end_to_end/e2e/tests/datasets/dataset-version-delete-counters.spec.ts, both@t2-cuj @area:datasets @cap:datasets.version-history-view, and both drive the ungrouped write path (nobatch_group_id, so the latest version is mutated in place instead of a new one being cut). Writes go through the backend client; the figures are then read back off the UI — the disagreement between the two surfaces is the thing worth catching.1.
Deleting items moves only the total and deleted counters of the version it mutates, and the Version history tab renders themSeeds via the
datasetfixture (3 items,v1), adds 3 more, deletes 2 by id, then re-sends the identical delete. After every step it asserts the exact quadruple, that the version id did not change and no second version appeared, and thatitems_totalequals the item ids the dataset actually holds —{3,3,0,0}→{6,6,0,0}→{4,6,0,2}, unchanged on the repeat. The repeat also asserts204, not404: thedeletedCount == 0short-circuit has to fire before the counter update, or a repeated delete writes a phantom delta. Finally the Version history tab must render Item count4and exactly two change tags,+ 6and− 2— a~tag appearing there is a failure.Result: passed.
2.
Interleaved inserts, upserts and deletes keep the version counters equal to the items the dataset holdsA single operation can't catch a delta applied to the wrong column — after one write the numbers still look plausible. This drives insert 4 → delete 2 → insert 3 → upsert 2 stored ids with new content → delete 1, all into
v1, asserting the full quadruple and the stored id set after each:{3,3,0,0}→{7,7,0,0}→{5,7,0,2}→{8,10,0,2}→{8,10,2,2}→{7,10,2,3}. Then the tab must show Item count7and exactly three tags,+ 10 ~ 2 − 3.Result: passed.
Verification
Run against
https://pr-7705.dev.comet.com(OPIK_DEPLOYMENT=oss, workspacedefault). The wholetests/datasets/directory was run, not just the new file, becauseDatasetItemsPageis shared — the 5 pre-existing specs still pass.Two things a reviewer should know about that run:
makeBackendClientrequires an API key for any non-localhost host, so the run passed a placeholderOPIK_API_KEYat an OSS install that has no auth. That's a property of running the OSS suite against a remote OSS deployment, not something the specs need.4to5and the spec failed at exactly that line. Reverted before committing.What I deliberately did not write
One candidate was dropped: concurrent batch inserts and deletes into one version (8 workers × 5 batches × 10 items →
items_totalexactly 401; concurrent insert+delete → 401/441/0/40). It passed by hand on the PR environment, but it was marked weak by the exploration and I agree:withDatasetVersionLockstill serialises these API paths, so the assertion would be about end-to-end exactness rather than the lock-free property the PR claims — the DAO-level proof is the PR's own unit test — and a wall-clock-sensitive fan-out is a flake risk the suite shouldn't take on for that. If the team wants a single-threaded stand-in with similar value, the deterministic one is a >1000-row CSV upload (csvBatchSizeis 1000), which drives several sequential increments into one version through a real product path.Also not proposed: the
COALESCE(col, 0)NULL-counter guard (not producible through the API — everyINSERT INTO dataset_versionsnames the columns and everyDatasetVersion.builder()sets them; the PR's own unit test forces it with raw SQL), and a seconddatasets.bulk-delete-itemsspec (already covered at t3-nightly; the counter assertions belong under the uncovered capability instead).Supporting changes
core/backend/client.ts—getDatasetVersions,listDatasetItemIds,upsertDatasetItemsIntoLatestVersion,deleteDatasetItemsByIds. The delete helper returns the HTTP status rather thanvoidso a caller can tell a specified no-op204from a404.pom/dataset-items.page.ts— Version history tab:openVersionHistory,versionHistoryRow,versionItemCount,versionChangeSummary,versionChangeTags,versionChangeTag. Cells are addressed by the sharedDataTable'sdata-cell-id, not by position, because column order is user-configurable.coverage/taxonomy.yaml— spec added to the area'sspecs:list,datasets.version-history-viewflipped tocovered: true, tier: t2-cujwith a note on what it does and does not assert.One merge note
mainhas moved on since this branch point: it now carriesdatasets/dataset-version-counters.spec.ts, which covers the insert side of the same capability (multi-batch and multi-threadedinsert(), adds/modified), and already hasversion-history-viewflipped to covered. That spec and this one are complementary — nothing here re-asserts the insert path — and the filenames deliberately don't collide. Expect small textual conflicts intaxonomy.yaml(thespecs:list, the capability line) and incore/backend/client.ts/pom/dataset-items.page.ts, wheremainhas near-identical versions ofgetDatasetVersions,listDatasetItemIds,openVersionHistoryandversionItemCount; takemain's and keep the additions here that it lacks (DatasetVersionRef.id, the write/delete helpers, the change-summary locators).Generated by the release QA side flow. Needs human review before merge.