[OPIK-7891] [BE] fix: count distinct item ids when sizing a dataset version - #7966
Conversation
github-actions
Bot
added
java
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
Drop the fall back to item.id() when datasetItemId is null. It looked defensive but was worse than nothing: the INSERT ten lines below binds item.datasetItemId().toString() with no null check, so a null cannot survive to be counted either way, and every caller normalizes datasetItemId before reaching here. All the fallback added was a second, divergent definition of item identity in the one place that must agree with what the INSERT writes. Addresses review feedback on #7966. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
|
Already covered by a test in this PR. The fix is a real user-facing one — also touches Backend (Java API / internal) Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. Re-checked after a push on 30 Aug 06:05 UTC. |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
✅ Test environment is now available! To configure additional Environment variables for your environment, run [Deploy Opik AdHoc Environment workflow] (https://github.com/comet-ml/comet-deployment/actions/workflows/deploy_opik_adhoc_env.yaml) Access Information
The deployment has completed successfully and the version has been verified. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
andrescrz
left a comment
andrescrz
left a comment
There was a problem hiding this comment.
LGTM, just a minor test assertion improvement to make them more reliable. Not a blocker.
Both OPIK-7891 tests checked stored.size() and left the row identities unasserted. On a de-duplication test that is the weakest possible check: the count is exactly what a bug is most likely to keep right. Keeping the wrong revision of the duplicate, or dropping the distinct row and keeping both duplicates, would each leave the size at 2 and pass. Now both name the rows that must survive and compare whole objects via the shared IGNORED_FIELDS_DATA_ITEM element comparator, so a wrong-winner bug fails. This also pins behaviour the tests previously only assumed: the last submitted revision of a repeated id is the one that survives. The itemsTotal assertion still ties the counter to stored.size() rather than a literal, which is only meaningful now that stored itself is pinned. Addresses review feedback on #7966. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
🟡 size/M
and removed
python
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>
|
Will rebase, after approval / or when #7705 gets merged first, FYI @thiagohora @andrescrz 
|
…ersion insertItems returned items.size() -- the raw list length, deliberately not a DB row count, because ClickHouse async inserts report 0 rows before commit. Reads collapse a repeated dataset_item_id to one row via LIMIT 1 BY, so a stable id appearing twice in one batch made the version total one higher than the rows the version actually holds. Every version total flows through this value: createFirstVersion feeds it straight to itemsTotal, and applyDelta sums added + edited + copied. So the fix belongs at the source -- count distinct stable ids, matching what the storage engine keeps. This corrects the mechanism recorded on the ticket. The reported cause was asynchronous ClickHouse visibility racing concurrent batches on the append path; it is neither a race nor on the append path. One single-threaded request reproduces it, and the appending path already classifies a cross-batch duplicate correctly via countExistingItemIds -- verified by a test that passes with and without this change. num_threads was a red herring: the SDK's content-hash dedup drops same-content duplicates before batching, so a duplicate id only survives into a request when the content differs. Tests cover both shapes: a duplicate inside the version-creating batch (fails without this fix) and a duplicate spanning two batches in one batch_group_id (passes either way, pinning the append path's behaviour). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drop the fall back to item.id() when datasetItemId is null. It looked defensive but was worse than nothing: the INSERT ten lines below binds item.datasetItemId().toString() with no null check, so a null cannot survive to be counted either way, and every caller normalizes datasetItemId before reaching here. All the fallback added was a second, divergent definition of item identity in the one place that must agree with what the INSERT writes. Addresses review feedback on #7966. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both OPIK-7891 tests checked stored.size() and left the row identities unasserted. On a de-duplication test that is the weakest possible check: the count is exactly what a bug is most likely to keep right. Keeping the wrong revision of the duplicate, or dropping the distinct row and keeping both duplicates, would each leave the size at 2 and pass. Now both name the rows that must survive and compare whole objects via the shared IGNORED_FIELDS_DATA_ITEM element comparator, so a wrong-winner bug fails. This also pins behaviour the tests previously only assumed: the last submitted revision of a repeated id is the one that survives. The itemsTotal assertion still ties the counter to stored.size() rather than a literal, which is only meaningful now that stored itself is pinned. Addresses review feedback on #7966. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
hasSize alone asserts a count and nothing about identity, so a bug that preserves the count passes. Called out on #7966, where both new de-duplication tests checked stored.size() and left the row identities unasserted -- the count is exactly what a de-dup bug is most likely to get right. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
74d011a to
77a681e
Compare
andrescrz
left a comment
andrescrz
left a comment
There was a problem hiding this comment.
One advice on the testing.md rules, but not a blocker. It can go on a follow-up PR.
| rather than to a literal — but it is only as strong as the assertion on `stored` itself. Pin the | ||
| contents first, then tie the counter to them. | ||
|
|
||
| Reuse the shared ignore-field constants (`IGNORED_FIELDS_DATA_ITEM` and friends) rather than |
There was a problem hiding this comment.
Not only should reuse the constant, many times there's an existing helper method or even helper class which should be used (generally delegates to the constant). That way we keep the assertions logic centralised.
There was a problem hiding this comment.
Agreed — and the point generalises past the constant. Tracked as OPIK-8181 so this PR can merge without growing further.
The dataset item tests are the clearest instance: IGNORED_FIELDS_DATA_ITEM is declared twice (DatasetsResourceTest:219 and, independently, ExperimentAggregatesIntegrationTest:130), DatasetVersionResourceTest imports it across package boundaries from a sibling resource test class, and ~10 call sites repeat the comparator chain by hand. Two copies of the list can drift silently.
The follow-up extracts a DatasetItemAssertions under api/resources/utils/ mirroring TraceAssertions, migrates the call sites, and reworks the skill-doc guidance to say helper-method-or-class over inlined constant. Doing it in a separate PR rather than here because it touches three large test classes and needs the two constant declarations diffed and reconciled deliberately — that reconciliation is its own reviewable decision, not a drive-by in a counter-correctness fix.
Note the hasSize doc section this thread is anchored on is no longer on the branch; restoring it, phrased around helper reuse, is part of the follow-up.
There was a problem hiding this comment.
@JetoPistola as general feedback, let's not create a ticket for small follow-up comments like this.
We send the PR or not, but as part of the same tickets.
We can't maintain a backlog of PR follow-up tickets, it's too much burden. In addition, it's not the purpose of the backlog.
There was a problem hiding this comment.
@andrescrz - thanks for the feedback, appreciate it
Agreed - we shouldn't overload the backlog, especially with PR follow-up tickets (Like this comment) - avoiding ticket bloat, makes sense.
Perhaps my main concern was that touching three large test classes and reconciling the constant drift felt a bit too heavy for a drive-by change, in this correctness PR
That said, there were alternative approaches for me to take:
- Absorb It Immediately (The "Do It Now" Rule) - postponing merge, to avoid the follow up PR
- The Fast-Follow PR Without Jira - NA ticket - create draft, and iterate now/later
- Code-Level TODOs - perhaps this is an out dated approach - invisible graveyard like, for that we look in the code, or scan TODOs manually/automatically...
- Using the same ticket number - while possible, issue rises with the jira status dance, for the label status, switching its value unexpectedly and having reference to multiple PRs from a jira ticket - which is why a separate ticket felt like the default path. Additional reason is scoping sessions - similar to pull requests - to keep them small and targeted
This time, already spun the jira ticket. Future wise - hope that will adjust the workflow, to keep things leaner
5 participants
Details
DatasetItemVersionDAO.insertItemsreturneditems.size()— the raw list length. That is deliberate, not an oversight: ClickHouse async inserts report 0 rows before commit, so the code returns the count of what it handed in rather than trustinggetRowsUpdated. But reads collapse a repeateddataset_item_idto a single row viaLIMIT 1 BY, so a stable id appearing twice in one batch made the stored version total one higher than the rows the version actually holds.Every version total flows through that return value —
createFirstVersionfeeds it straight intoitemsTotal, andapplyDeltasums added + edited + copied — so the fix belongs at the source: count distinct stable ids, matching what the storage engine keeps.DISTINCTdatasetItemIdininsertItems— the same field theINSERTbelow binds, so counting and writing share one definition of item identity. (An earlier revision added anitem.id()fallback for a nulldatasetItemId; it was removed after review. TheINSERTbindsdatasetItemId().toString()with no null check, so a null cannot survive to be counted either way, and every caller normalizes the field first.)This corrects the mechanism recorded on the ticket
The ticket described this as asynchronous ClickHouse visibility racing concurrent batches on the append path, reachable only via
num_threads>1. Investigation shows it is none of those things:num_threads>1countExistingItemIds)insertItems→createFirstVersion)The append path is genuinely correct —
countExistingItemIdsclassifies a cross-batch duplicate as an update. A test in this PR covers that case and passes both with and without the fix, which is what rules the append path out.num_threadswas a red herring: the SDK's content-hash dedup drops same-content duplicates before batching, so a duplicate id only survives into a request when the content differs. The reporter's parallel runs were simply the ones where such an id landed in the version-creating batch.Change checklist
User-facing: the "Item count" in Datasets → dataset → Version history (and
dataset_items_countin the SDK) now agrees with the rows the version holds. No API shape change.Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
Two tests added, deliberately covering both shapes, and each was run with and without the fix:
items_total3, rows stored 2batch_group_idThe first is the regression test. The second is a negative control: it pins the append path's existing correct behaviour and is the evidence that the reported async-visibility mechanism is not the cause. Both assert
items_totalagainst the rows actually returned for the version, rather than against a hardcoded number, so a future change that breaks one but not the other still fails.Not verified:
items_totalis already wrong. Reconciliation is tracked separately under OPIK_7804.Documentation
N/A — internal counter correctness fix with no API or configuration surface.