[OPIK-8175] [BE] perf: skip the dataset item-count scan when a version supplies items_total - #8075
Conversation
…n supplies items_total enrichDatasetWithAdditionalInformation ran an O(N) count(DISTINCT id) over dataset_items unconditionally, then discarded the result whenever dataset versioning supplied itemsTotal. Move the latest-version lookup ahead of the count and narrow the count query to the datasets that actually need the legacy fallback, skipping it entirely when none do. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
java
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
… count versionItemsTotal accepted any non-null itemsTotal as authoritative, including the ITEMS_TOTAL_NOT_MIGRATED (-1) sentinel written by migration 000046 for versions awaiting backfill. main had the same hole, but narrowing the fallback made it reachable: the dataset was excluded from the count query entirely, so -1 became the only possible answer instead of merely a wasted query. Also drop the eager DatasetItemSummary.empty allocation that getOrDefault evaluated on every dataset regardless of whether the map had a match. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🔄 Test environment deployment process has started Phase 1: Deploying base version You can monitor the progress here. |
|
This change looks worth a test. The item-count scan is now narrowed to the datasets that actually fall back, which is value-neutral by design — but Would target What it would check
Deploying a test environment for this PR and exploring it — results will follow in a comment. Not testable yet. The second change is behavioural, not perf: 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 04 Sep 09:32 UTC. |
|
✅ 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. |
|
Minor PR-description cleanup I noticed while reviewing the current head: DatasetServiceEnrichmentTest now contains 8 @test methods, while the description still says 7 cases / tests=7. I saw the sentinel case was already caught and addressed in the review thread, so this is mainly about keeping the PR description aligned with the final implementation. |
|
🌙 Nightly cleanup: The test environment for this PR ( |
OPIK_8176 landed on main and parallelised the four enrichment queries into a single Mono.zip, which requires the dataset_items count to be issued unconditionally -- the query this branch removes. Resolution keeps both: - The two version-independent ClickHouse lookups and the version lookup stay concurrent in the zip. The dataset_items count is chained off the version result via flatMap, so it is still narrowed to the fallback subset and still skipped entirely when that subset is empty. - fetchDatasetItemSummaries now returns Mono<Map<...>> instead of blocking, so it composes inside the reactive chain rather than calling toStream(). - Adopted main's thread-safety discipline: workspaceId/userName resolved on the request thread, and getOrDefault instead of computeIfAbsent on shared maps. Test files were an add/add conflict; both suites are kept. Main's concurrency test now gates on the two version-independent lookups rather than three, since the item count is deliberately no longer among them, and a new test pins that the count is not issued until versions have resolved.
…solution Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… alone Chaining fetchDatasetItemSummaries off the whole Mono.zip made it wait on the experiment and optimization summaries, which it has no dependency on: the zip only emits once all three sources complete, so a fallback-heavy page serialized a ClickHouse round trip that could have overlapped. cache() the version lookup so one execution feeds both the zip and the count chain, then zip the chain alongside the other two lookups. The count is issued as soon as the versions resolve, and the narrowed id set is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks @amebaleon — both points were valid when you wrote them, and both are now resolved in the description.
Worth flagging one thing that landed after your review: the merge resolution had the fallback count chained off the whole 🤖 Reply posted via /address-github-pr-comments |
The Mockito answer fires when findDatasetItemSummaryByDatasetIds is invoked, not when the returned Flux is subscribed, so itemCountSubscribed overstated the synchronization point. Rename to itemCountQueryIssued and spell out in the comment that invocation -- the point the narrowed id set is handed over -- is the property under test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…dination Hand-rolled CountDownLatch waiting had to get the deadline and interrupt handling right itself, and a missed expectation left a worker parked on a bare await() -- the failure mode that turns into a hung CI job. Awaitility now owns every wait in the non-blocking test: untilTrue for the assertion, and the same for the gate holding the unrelated summaries, so awaitQuietly is gone. A timeout now reports the named condition instead of a bare assertion failure. Also import AtomicBoolean rather than spelling it fully qualified. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Backend Tests - Integration Group 14 51 files + 8 51 suites +8 3m 25s ⏱️ -49s Results for commit 5528323. ± Comparison against base commit 0b904ae. This pull request removes 31 and adds 41 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
|
🔄 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. |
|
Explored this PR on its own test environment. Worked through all 5 items on the triage Per-item results
2 flows look worth a permanent test:
Writing the spec now; a draft PR will follow. Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. |
5 participants
Details
enrichDatasetWithAdditionalInformationran acount(DISTINCT id)overdataset_itemsunconditionally, then discarded the result whenever dataset versioning supplieditemsTotal. The count is O(N) in the dataset's item count — the sort-key prefix prunes to the right dataset, but every id in that range still has to be read and hashed, since duplicates pending merge must be collapsed. This resolves the item-count source from the latest version first, and issues thedataset_itemsquery only for the datasets that actually need the legacy fallback.count(DISTINCT id)againstdataset_itemsat all.findById,findByNameDetailed, and both list paths) funnel through this single method, so the dataset list page and the SDKget_or_create_datasetpath are both covered.ITEMS_TOTAL_NOT_MIGRATED(-1) sentinel written by migration000046is treated as unavailable rather than as an authoritative count, so an un-backfilled dataset falls back to the legacy count instead of reporting-1. That hole pre-dated this branch, but narrowing the fallback would have made-1the only reachable answer rather than merely a wasted query.Interaction with the concurrent-enrichment change. OPIK_8176 landed on main first and zips the four enrichment lookups together, which requires the
dataset_itemscount to be issued unconditionally. This branch keeps both properties: the two version-independent ClickHouse lookups and the version lookup stay concurrent in theMono.zip, while the item count is chained off the version result viaflatMap— so it is still narrowed and still skippable.fetchDatasetItemSummariesreturns aMonorather than blocking viatoStream(), so it composes inside the reactive chain, and main's thread-safety discipline is preserved (workspace/user resolved on the request thread;getOrDefaultrather thancomputeIfAbsenton the shared maps).Counts are otherwise unchanged. The previous code selected the version total when
flag && version != null && itemsTotal != nulland fell back to the DAO count otherwise;versionItemsTotalreturns non-null under exactly that condition (plus the new sentinel guard), and the fallback fires precisely when it returns null. Datasets omitted from the query are, by construction, those whose count comes from their version, so the omitted row would have been discarded anyway. A dataset in the fallback subset with nodataset_itemsrows still resolves to0.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
DatasetServiceEnrichmentTestnow holds 21 cases: the 11 concurrency/defaulting cases from the enrichment change already on main, plus 10 covering item-count source selection and its scheduling.Item-count source selection:
findDatasetItemSummaryByDatasetIdsis never called, and counts come from the versions.dataset_itemscount.itemsTotal— falls back to the count.ITEMS_TOTAL_NOT_MIGRATED(-1) sentinel — falls back to the count rather than reporting-1.0.findById) — skips the query for a versioned dataset.Scheduling:
The concurrency test inherited from main was updated: it gated on all three ClickHouse lookups subscribing before any completed, which this change makes structurally impossible, since the item count now runs after the version lookup by design. It gates on the two version-independent lookups instead, and the two scheduling tests above cover what it no longer can.
Commands run, from
apps/opik-backend:Each behaviour was checked to be genuinely pinned rather than merely passing, by reverting the production change and confirming the matching test fails: the sentinel guard, the unconditional zip (fails exactly the four ordering/skip/narrowing tests), and chaining the count off the whole zip instead of the version lookup (fails only the non-blocking test).
Not run locally: the
DatasetsResourceTestandDatasetVersionResourceTestintegration suites, which need containers. They already cover both count sources end-to-end through the public resource layer —DatasetVersionResourceTestasserts the list API takes the count from the latest version (flag on, real containers), andDatasetsResourceTestasserts the legacy count on unversioned datasets acrossfindByIdentifier,findById, and list — so they are the meaningful end-to-end gate for this change in CI.Documentation
N/A — internal query-path optimization with no user-facing or API surface change.