Skip to content

Commit 74d011a

Browse files
test(datasets): drop the nondeterministic duplicate-winner assertion
Every row in a batch is written with the same now64(9), and the read dedupes with ORDER BY dataset_item_id DESC, last_updated_at DESC LIMIT 1 BY dataset_item_id -- no tie-breaker. Which revision of a repeated id survives is therefore unspecified, so asserting that the later payload wins was pinning an accident. Assert what is guaranteed instead: exactly one row per distinct id, and the distinct item's content intact. Also corrects a comment that described insertItems' pre-fix return value in the present tense. Addresses review feedback on #7966. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent bad9213 commit 74d011a

1 file changed

Lines changed: 24 additions & 19 deletions

File tree

apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/DatasetVersionResourceTest.java

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3790,21 +3790,14 @@ void putItems__whenCreatingBatchRepeatsStableId__thenCountedOnce() {
37903790
var datasetId = createDataset(UUID.randomUUID().toString());
37913791

37923792
// The SDK's parallel upload sends every batch under one batch_group_id. The batch that
3793-
// arrives first CREATES the version, and that path derives itemsTotal from insertItems,
3794-
// which returns items.size() -- the raw list length, deliberately not a DB row count
3795-
// (ClickHouse async inserts report 0 before commit). A stable id repeated inside that
3796-
// first batch is therefore counted twice, while ClickHouse collapses it to one row.
3793+
// arrives first CREATES the version, and that path derives itemsTotal from insertItems.
3794+
// That used to return items.size() -- the raw list length, deliberately not a DB row count
3795+
// (ClickHouse async inserts report 0 before commit) -- so a stable id repeated inside the
3796+
// first batch was counted twice while ClickHouse collapsed it to one row. It now counts
3797+
// distinct dataset_item_ids, which is what this test pins.
37973798
var duplicatedId = TestIdGeneratorFactory.create().generateId();
37983799
var distinctId = TestIdGeneratorFactory.create().generateId();
37993800

3800-
// The repeated id wins with its LAST submitted content: ClickHouse keeps one row per
3801-
// dataset_item_id and reads take the newest, so "second" survives and "first" does not.
3802-
var winningDuplicate = DatasetItem.builder()
3803-
.id(duplicatedId)
3804-
.datasetItemId(duplicatedId)
3805-
.source(DatasetItemSource.SDK)
3806-
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"second\"")))
3807-
.build();
38083801
var distinctItem = DatasetItem.builder()
38093802
.id(distinctId)
38103803
.datasetItemId(distinctId)
@@ -3818,7 +3811,11 @@ void putItems__whenCreatingBatchRepeatsStableId__thenCountedOnce() {
38183811
.source(DatasetItemSource.SDK)
38193812
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"first\"")))
38203813
.build(),
3821-
winningDuplicate,
3814+
DatasetItem.builder()
3815+
.id(duplicatedId)
3816+
.source(DatasetItemSource.SDK)
3817+
.data(Map.of("value", JsonUtils.getJsonNodeFromString("\"second\"")))
3818+
.build(),
38223819
distinctItem);
38233820

38243821
datasetResourceClient.createDatasetItems(DatasetItemBatch.builder()
@@ -3828,15 +3825,23 @@ void putItems__whenCreatingBatchRepeatsStableId__thenCountedOnce() {
38283825
.build(), TEST_WORKSPACE, API_KEY);
38293826

38303827
var version = getLatestVersion(datasetId);
3831-
3832-
// Assert on the rows themselves, not just how many: a bug that kept the wrong revision
3833-
// of the duplicate, or dropped the distinct item and kept both duplicates, would leave
3834-
// the count at 2 and slip through a size-only check.
38353828
var stored = datasetResourceClient.getDatasetItems(
38363829
datasetId, 1, 100, version.versionHash(), API_KEY, TEST_WORKSPACE).content();
3830+
3831+
// Which revision of the repeated id survives is deliberately NOT asserted: every row in a
3832+
// batch is written with the same now64(9), and the read dedupes with
3833+
// `ORDER BY dataset_item_id DESC, last_updated_at DESC LIMIT 1 BY dataset_item_id` -- no
3834+
// tie-breaker, so either payload may win. Pinning "second" would be asserting an accident.
3835+
// What is guaranteed, and what the fix is about: exactly one row per distinct id, and a
3836+
// counter that agrees with it.
3837+
assertThat(stored).extracting(DatasetItem::id)
3838+
.containsExactlyInAnyOrder(duplicatedId, distinctId);
38373839
assertThat(stored)
3838-
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
3839-
.containsExactlyInAnyOrder(winningDuplicate, distinctItem);
3840+
.filteredOn(item -> distinctId.equals(item.id()))
3841+
.singleElement()
3842+
.usingRecursiveComparison()
3843+
.ignoringFields(IGNORED_FIELDS_DATA_ITEM)
3844+
.isEqualTo(distinctItem);
38403845

38413846
// items_total must agree with what is actually stored.
38423847
assertThat(version.itemsTotal()).isEqualTo(stored.size());

0 commit comments

Comments
 (0)