[OPIK-4727] [BE] fix: address PR #5554 review feedback#5725
[OPIK-4727] [BE] fix: address PR #5554 review feedback#5725itamargolan wants to merge 7 commits intomainfrom
Conversation
- Consolidate datasetId/datasetIds in OptimizationSearchCriteria to use only datasetIds (Collection<UUID>), wrapping the API's single dataset_id query param into a singleton list - Add dataset_item_count to experiment_aggregates table via ClickHouse migration and populate it during aggregation computation - Add unit test for execution policy resolution without dataset versioning Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
added
java
📋 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 |
📋 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 |
📋 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 |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
...nd/src/main/java/com/comet/opik/domain/experiments/aggregations/ExperimentAggregatesDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/OptimizationsResource.java
Show resolved
Hide resolved
...ibase/db-app-analytics/migrations/000073_add_dataset_item_count_to_experiment_aggregates.sql
Show resolved
Hide resolved
Python SDK E2E Tests Results (Python 3.10)238 tests ±0 236 ✅ ±0 9m 45s ⏱️ +52s Results for commit 8278318. ± Comparison against base commit 73e44f3. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Python SDK E2E Tests Results (Python 3.14)238 tests ±0 235 ✅ - 1 8m 45s ⏱️ -27s For more details on these failures, see this check. Results for commit a7b6330. ± Comparison against base commit 73e44f3. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Python SDK E2E Tests Results (Python 3.11)238 tests ±0 236 ✅ ±0 9m 17s ⏱️ +4s Results for commit 8278318. ± Comparison against base commit 73e44f3. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Python SDK E2E Tests Results (Python 3.13)238 tests ±0 236 ✅ ±0 9m 13s ⏱️ +19s Results for commit 8278318. ± Comparison against base commit 73e44f3. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…act count helper - Intersect explicit dataset_id with name-resolved IDs in resolveDatasetNameFilter instead of unconditionally overwriting (fixes logical bug when both filters provided) - Extract queryExperimentCount helper following existing queryExperimentAggregation pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationService.java
Outdated
Show resolved
Hide resolved
Tests cover: blank datasetName (skips resolution), no matches (empty page), successful resolution, intersection when both datasetId and datasetName are set, and empty intersection returning empty page. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Python SDK E2E Tests Results (Python 3.12)238 tests ±0 236 ✅ ±0 9m 12s ⏱️ -3s Results for commit 8278318. ± Comparison against base commit 73e44f3. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
andrescrz
left a comment
andrescrz
left a comment
There was a problem hiding this comment.
Thanks for putting this together. My main comments:
- Avoid duplicated queries that can be done in one go.
- Mindful of when to use final or not.
- Testing strategy, make sure you cover your changes from the resource testing. That should be the most important coverage. Optionally, you can add extra unit tests if any value.
The rest is minor comments, feel free to ignore.
...nd/src/main/java/com/comet/opik/domain/experiments/aggregations/ExperimentAggregatesDAO.java
Outdated
Show resolved
Hide resolved
...nd/src/main/java/com/comet/opik/domain/experiments/aggregations/ExperimentAggregatesDAO.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationService.java
Outdated
Show resolved
Hide resolved
apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
As small note: we follow the convention of having black box testing from the resources and avoid unnecessary unit tests. That means that we don't add unit tests for things already covered from there, typically the service logic. Having said that, feel free to keep this test class if it adds value.
…eedback-fixes # Conflicts: # apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationDAO.java # apps/opik-backend/src/main/java/com/comet/opik/domain/experiments/aggregations/ExperimentAggregatesDAO.java
…MIT 1 BY, narrow SELECTs - Replace all FINAL usages in ExperimentAggregatesDAO with ORDER BY/LIMIT 1 BY - Narrow SELECT * in dedup subqueries to only needed columns (traces, feedback_scores, authored_feedback_scores, dataset_item_versions, experiments) - Consolidate dataset_id (singular) into dataset_ids (plural) across all DAOs - Consolidate id/ids_list/experiment_ids into single experiment_ids field in AggregationBranchCountsCriteria - Fix assertion_results_per_trace CTE to cover both aggregated and raw paths - Update experimentDaoFindMatchesAggregates test for datasetItemCount Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentDAO.java
Outdated
Show resolved
Hide resolved
…sertion regression test - Rename migration 000071 to 000073 to fix prefix conflict with assertion_results table migration - Extract resolveDatasetIds() into ExperimentSearchCriteria, resolve at TargetProjectsCriteria construction - Update ExperimentSearchCriteriaBinder and ExperimentDAO to use the shared method - Add integration test for aggregated experiment assertion results in compare endpoint Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| List<FilterStrategy> filterStrategies, | ||
| boolean bindEntityType) { | ||
|
|
||
| // Bind basic criteria | ||
| Optional.ofNullable(criteria.datasetId()) | ||
| .ifPresent(datasetId -> statement.bind("dataset_id", datasetId)); | ||
| // Bind basic criteria — dataset_id singular is consolidated into dataset_ids | ||
| Optional.ofNullable(criteria.resolveDatasetIds()) | ||
| .ifPresent(datasetIds -> statement.bind("dataset_ids", datasetIds.toArray(UUID[]::new))); | ||
| Optional.ofNullable(criteria.name()) | ||
| .ifPresent(name -> statement.bind("name", name)); | ||
| Optional.ofNullable(criteria.datasetIds()) | ||
| .ifPresent(datasetIds -> statement.bind("dataset_ids", datasetIds.toArray(UUID[]::new))); | ||
| Optional.ofNullable(criteria.promptId()) |
There was a problem hiding this comment.
Optional.ofNullable(criteria.resolveDatasetIds()) no longer chains to ifPresent so dataset_ids isn't bound and dataset filtering breaks; should we restore the binding (and the same for ExperimentDAO's Optional.ofNullable(criteria.datasetIds())) with Optional.ofNullable(criteria.resolveDatasetIds()).ifPresent(ds -> statement.bind("dataset_ids", ds.toArray(UUID[]::new)))
Finding type: reduce code duplication | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In
apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentSearchCriteriaBinder.java
around lines 36 to 44, the bindSearchCriteria method calls
Optional.ofNullable(criteria.resolveDatasetIds()) but fails to chain .ifPresent(...), so
"dataset_ids" is never bound to the statement. Restore the binding by changing that line
to Optional.ofNullable(criteria.resolveDatasetIds()).ifPresent(ds ->
statement.bind("dataset_ids", ds.toArray(UUID[]::new))). Also apply the same fix in
apps/opik-backend/src/main/java/com/comet/opik/db/ExperimentDAO.java around line 2520
where Optional.ofNullable(criteria.datasetIds()) is missing an ifPresent bind; add
.ifPresent(ds -> statement.bind("dataset_ids", ds.toArray(UUID[]::new))).
…CTs, deduplicate resolveDatasetIds - Remove pointless WHERE 1=1 in SELECT_EXPERIMENT_BY_ID and GET_TRACES_DATA (no conditional filters follow) - Remove duplicate dataset_ids filter in OptimizationDAO outer WHERE (already filtered in inner dedup subquery) - Use criteria.resolveDatasetIds() in buildCountTemplate instead of duplicating the resolution logic - Narrow SELECT * to only needed columns in 4 experiment_aggregates JOIN subqueries (dataset_item_versions queries) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| SELECT id, workspace_id, dataset_id, dataset_version_id | ||
| FROM experiment_aggregates | ||
| WHERE workspace_id = :workspace_id | ||
| ORDER BY (workspace_id, dataset_id, id) DESC, last_updated_at DESC | ||
| LIMIT 1 BY workspace_id, dataset_id, id |
There was a problem hiding this comment.
The inner join that fetches the latest experiment aggregate (SELECT id, workspace_id, dataset_id, dataset_version_id … LIMIT 1 BY workspace_id, dataset_id, id) is duplicated in SELECT_DATASET_ITEM_VERSIONS_WITH_EXPERIMENT_ITEMS_COUNT and SELECT_DATASET_ITEM_VERSIONS_WITH_EXPERIMENT_ITEMS, should we extract it into a shared CTE/helper?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
| private ST buildCountTemplate(ExperimentSearchCriteria criteria, String workspaceId) { | ||
| var template = getSTWithLogComment(FIND_COUNT_FROM_AGGREGATES, "count_experiments_from_aggregates", | ||
| workspaceId, ""); | ||
| Optional.ofNullable(criteria.datasetId()) | ||
| .ifPresent(datasetId -> template.add("dataset_id", datasetId)); | ||
| Optional.ofNullable(criteria.resolveDatasetIds()) | ||
| .ifPresent(datasetIds -> template.add("dataset_ids", datasetIds)); | ||
| Optional.ofNullable(criteria.name()) | ||
| .ifPresent(name -> template.add("name", name)); |
There was a problem hiding this comment.
Optional.ofNullable(criteria.resolveDatasetIds()) discards its result so dataset IDs aren't bound; should we follow with .ifPresent(ids -> template.add("dataset_ids", ids))?
Finding type: prefer simpler algorithms | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In
apps/opik-backend/src/main/java/com/comet/opik/domain/experiments/aggregations/ExperimentAggregatesDAO.java
around lines 2569 to 2575, the buildCountTemplate method contains a standalone
Optional.ofNullable(criteria.resolveDatasetIds()) call whose result is discarded. This
means dataset IDs are not bound into the SQL template and the count query ignores
dataset filters. Replace that stray line by chaining .ifPresent(ids ->
template.add("dataset_ids", ids)) so the resolved dataset IDs are added to the template
(i.e. Optional.ofNullable(criteria.resolveDatasetIds()).ifPresent(ids ->
template.add("dataset_ids", ids))). Ensure the change compiles and add a
unit/integration test to verify countTotal respects datasetIds where applicable.
2 participants
Details
Addresses backend review feedback from @andrescrz on PR #5554 (optimizer screens face-lift).
Changes:
datasetId/datasetIdsfilter — Removed singulardatasetIdfromOptimizationSearchCriteria. The API's@QueryParam("dataset_id")is now wrapped intoList.of()fordatasetIds. Eliminates dual-filter inOptimizationDAO.dataset_item_countto experiment aggregates — New ClickHouse migration + DAO changes to compute and store distinct dataset item counts inexperiment_aggregates.ExperimentItemServiceTestverifying execution policy resolution without dataset versioning (on-prem).Not changed (with rationale):
resolveDatasetNameFilter—dataset_versionshas nonamecolumn; current approach is correct.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
mvn compile -DskipTests— compilation verifiedmvn spotless:apply— formatting appliedExperimentItemServiceTestunit test passesDocumentation
No documentation changes needed — internal backend refactoring.