Skip to content

[OPIK-4727] [BE] fix: address PR #5554 review feedback #5725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
itamargolan wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

[OPIK-4727] [BE] fix: address PR #5554 review feedback #5725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d84cca7
[OPIK-4727] [BE] fix: address PR review feedback for optimizer screens
itamargolan Mar 18, 2026
d318374
fix: address Baz review feedback — intersect dataset filters and extr…
itamargolan Mar 18, 2026
2f621cd
test: add unit tests for resolveDatasetNameFilter in OptimizationService
itamargolan Mar 18, 2026
da55f5f
Merge remote-tracking branch 'origin/main' into itamar/OPIK-4727-pr-f…
itamargolan Mar 19, 2026
8278318
fix: address PR review - two-layer dedup pattern, remove redundant LI…
itamargolan Mar 20, 2026
abd6b8b
fix: migration prefix conflict, deduplicate resolveDatasetIds, add as…
itamargolan Mar 20, 2026
a7b6330
fix: clean up review response — remove redundant filters, narrow SELE…
itamargolan Mar 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions apps/opik-backend/src/main/java/com/comet/opik/api/ExperimentSearchCriteria.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import java.util.Collection;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.UUID;

Expand All @@ -16,4 +17,15 @@ public record ExperimentSearchCriteria(String name, UUID datasetId, @NonNull Ent
boolean datasetDeleted, Collection<UUID> datasetIds, UUID promptId, List<SortingField> sortingFields,
UUID optimizationId, Set<ExperimentType> types, List<? extends Filter> filters,
Set<UUID> experimentIds, UUID projectId, boolean projectDeleted) {

/**
* Resolves dataset IDs by preferring the plural {@code datasetIds} field,
* falling back to wrapping the singular {@code datasetId} in a list.
*/
public Collection<UUID> resolveDatasetIds() {
return Optional.ofNullable(datasetIds)
.orElseGet(() -> Optional.ofNullable(datasetId)
.map(List::of)
.orElse(null));
}
}
2 changes: 1 addition & 1 deletion ...pik-backend/src/main/java/com/comet/opik/api/resources/v1/priv/OptimizationsResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public Response find(
OptimizationFilter.LIST_TYPE_REFERENCE);

var searchCriteria = OptimizationSearchCriteria.builder()
.datasetId(datasetId)
.datasetIds(datasetId != null ? List.of(datasetId) : null)
.name(name)
.datasetName(datasetName)
.datasetDeleted(datasetDeleted)
Expand Down
6 changes: 5 additions & 1 deletion apps/opik-backend/src/main/java/com/comet/opik/domain/DatasetItemVersionDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,7 +1060,11 @@ ORDER BY (workspace_id, project_id, entity_id, id) DESC, last_updated_at DESC
WHERE entity_type = 'trace'
AND workspace_id = :workspace_id
<if(has_target_projects)>AND project_id IN :target_project_ids<endif>
AND entity_id IN (SELECT trace_id FROM experiment_items_final)
AND entity_id IN (
SELECT trace_id FROM experiment_items_final
UNION ALL
SELECT trace_id FROM experiment_item_aggr_trace_scope
)
GROUP BY entity_id
)
, dataset_items_aggr_resolved AS (
Expand Down
33 changes: 10 additions & 23 deletions apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,6 @@ class ExperimentDAO {
* Used to reduce traces, spans, and feedback_scores table scans.
*/
private record TargetProjectsCriteria(
UUID datasetId,
String name,
Collection<UUID> datasetIds,
UUID promptId,
Expand All @@ -102,7 +101,6 @@ private record TargetProjectsCriteria(

static TargetProjectsCriteria from(ExperimentGroupCriteria criteria) {
return new TargetProjectsCriteria(
null,
criteria.name(),
null,
null,
Expand All @@ -115,9 +113,8 @@ static TargetProjectsCriteria from(ExperimentGroupCriteria criteria) {

static TargetProjectsCriteria from(ExperimentSearchCriteria criteria) {
return new TargetProjectsCriteria(
criteria.datasetId(),
criteria.name(),
criteria.datasetIds(),
criteria.resolveDatasetIds(),
criteria.promptId(),
criteria.optimizationId(),
criteria.types(),
Expand Down Expand Up @@ -245,7 +242,6 @@ WITH experiments_resolved AS (
SELECT *
FROM experiments
WHERE workspace_id = :workspace_id
<if(dataset_id)> AND dataset_id = :dataset_id <endif>
<if(dataset_ids)> AND dataset_id IN :dataset_ids <endif>
<if(id)> AND id = :id <endif>
<if(ids_list)> AND id IN :ids_list <endif>
Expand Down Expand Up @@ -797,7 +793,6 @@ SELECT id, arrayConcat([prompt_id], mapKeys(prompt_versions)) AS prompt_ids, exp
SELECT *
FROM experiments
WHERE workspace_id = :workspace_id
<if(dataset_id)> AND dataset_id = :dataset_id <endif>
<if(dataset_ids)> AND dataset_id IN :dataset_ids <endif>
<if(experiment_ids)> AND id IN :experiment_ids <endif>
ORDER BY (workspace_id, dataset_id, id) DESC, last_updated_at DESC
Expand Down Expand Up @@ -1170,7 +1165,6 @@ id, arrayConcat([prompt_id], mapKeys(prompt_versions)) AS prompt_ids
SELECT *
FROM experiments
WHERE workspace_id = :workspace_id
<if(dataset_id)> AND dataset_id = :dataset_id <endif>
<if(dataset_ids)> AND dataset_id IN :dataset_ids <endif>
<if(experiment_ids)> AND id IN :experiment_ids <endif>
ORDER BY (workspace_id, dataset_id, id) DESC, last_updated_at DESC
Expand Down Expand Up @@ -1809,7 +1803,7 @@ private Publisher<? extends Result> insert(Experiment experiment, String executi
Mono<Experiment> getById(@NonNull UUID id) {
log.info("Getting experiment by id '{}'", id);
var limit = 1;
var aggregationCriteria = AggregationBranchCountsCriteria.builder().id(id).build();
var aggregationCriteria = AggregationBranchCountsCriteria.builder().experimentIds(Set.of(id)).build();

return getAggregationBranchCounts(aggregationCriteria)
.flatMap(counts -> {
Expand Down Expand Up @@ -1838,7 +1832,7 @@ Flux<Experiment> getByIds(@NonNull Set<UUID> ids) {
log.info("Getting experiment by ids '{}'", ids);

var aggregationCriteria = AggregationBranchCountsCriteria.builder()
.idsList(ids)
.experimentIds(ids)
.build();

return getAggregationBranchCounts(aggregationCriteria)
Expand Down Expand Up @@ -2150,12 +2144,10 @@ private Mono<AggregatedExperimentCounts> getAggregationBranchCounts(

private ST newFindTemplate(String query, ExperimentSearchCriteria criteria, String queryName, String workspaceId) {
var template = getSTWithLogComment(query, queryName, 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));
Optional.ofNullable(criteria.datasetIds())
.ifPresent(datasetIds -> template.add("dataset_ids", datasetIds));
Optional.ofNullable(criteria.promptId())
.ifPresent(promptId -> template.add("prompt_ids", promptId));
Optional.ofNullable(criteria.projectId())
Expand Down Expand Up @@ -2499,13 +2491,10 @@ private Mono<Set<UUID>> getTargetProjectIdsForExperiments(TargetProjectsCriteria
.flatMap(connection -> {
var template = TemplateUtils.newST(SELECT_TARGET_PROJECTS);

Optional.ofNullable(criteria.datasetId())
.ifPresent(datasetId -> template.add("dataset_id", datasetId));
Optional.ofNullable(criteria.name())
.ifPresent(name -> template.add("name", name));
Optional.ofNullable(criteria.datasetIds())
.filter(CollectionUtils::isNotEmpty)
.ifPresent(datasetIds -> template.add("dataset_ids", datasetIds));
Optional.ofNullable(criteria.name())
.ifPresent(name -> template.add("name", name));
Optional.ofNullable(criteria.promptId())
.ifPresent(promptId -> template.add("prompt_ids", promptId));
Optional.ofNullable(criteria.optimizationId())
Expand All @@ -2528,13 +2517,11 @@ private Mono<Set<UUID>> getTargetProjectIdsForExperiments(TargetProjectsCriteria
var statement = connection.createStatement(query);

// Bind the same criteria as the main query
Optional.ofNullable(criteria.datasetId())
.ifPresent(datasetId -> statement.bind("dataset_id", datasetId));
Optional.ofNullable(criteria.datasetIds())
.ifPresent(datasetIds -> statement.bind("dataset_ids",
datasetIds.toArray(UUID[]::new)));
Optional.ofNullable(criteria.name())
.ifPresent(name -> statement.bind("name", name));
Optional.ofNullable(criteria.datasetIds())
.filter(CollectionUtils::isNotEmpty)
.ifPresent(datasetIds -> statement.bind("dataset_ids", datasetIds.toArray(UUID[]::new)));
Optional.ofNullable(criteria.promptId())
.ifPresent(
promptId -> statement.bind("prompt_ids", List.of(promptId).toArray(UUID[]::new)));
Expand Down
8 changes: 3 additions & 5 deletions apps/opik-backend/src/main/java/com/comet/opik/domain/ExperimentSearchCriteriaBinder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,11 @@ public static void bindSearchCriteria(
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())
Comment on lines 36 to 44
Copy link
Copy Markdown
Contributor

@baz-reviewer baz-reviewer bot Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Fix in Cursor

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))).

.ifPresent(promptId -> statement.bind("prompt_ids", List.of(promptId).toArray(UUID[]::new)));
Optional.ofNullable(criteria.projectId())
Expand Down
8 changes: 0 additions & 8 deletions apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationDAO.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ WITH optimization_final AS (
SELECT *
FROM optimizations
WHERE workspace_id = :workspace_id
<if(dataset_id)>AND dataset_id = :dataset_id <endif>
<if(dataset_ids)>AND dataset_id IN :dataset_ids <endif>
<if(id)>AND id = :id <endif>
ORDER BY (workspace_id, dataset_id, id) DESC, last_updated_at DESC
Expand Down Expand Up @@ -384,7 +383,6 @@ WHERE isNotNull(weighted_score)
SELECT *
FROM optimizations
WHERE workspace_id = :workspace_id
<if(dataset_id)>AND dataset_id = :dataset_id <endif>
<if(dataset_ids)>AND dataset_id IN :dataset_ids <endif>
<if(id)>AND id = :id <endif>
ORDER BY (workspace_id, dataset_id, id) DESC, last_updated_at DESC
Expand Down Expand Up @@ -683,9 +681,6 @@ private void bindTemplateParams(ST template, OptimizationSearchCriteria searchCr
Optional.ofNullable(searchCriteria.datasetDeleted())
.ifPresent(datasetDeleted -> template.add("dataset_deleted", datasetDeleted.toString()));

Optional.ofNullable(searchCriteria.datasetId())
.ifPresent(datasetId -> template.add("dataset_id", datasetId));

Optional.ofNullable(searchCriteria.datasetIds())
.filter(ids -> !ids.isEmpty())
.ifPresent(datasetIds -> template.add("dataset_ids", datasetIds));
Expand All @@ -710,9 +705,6 @@ private void bindQueryParams(OptimizationSearchCriteria searchCriteria, Statemen
Optional.ofNullable(searchCriteria.datasetDeleted())
.ifPresent(datasetDeleted -> statement.bind("dataset_deleted", datasetDeleted));

Optional.ofNullable(searchCriteria.datasetId())
.ifPresent(datasetId -> statement.bind("dataset_id", datasetId));

Optional.ofNullable(searchCriteria.datasetIds())
.filter(ids -> !ids.isEmpty())
.ifPresent(datasetIds -> statement.bind("dataset_ids", datasetIds));
Expand Down
1 change: 0 additions & 1 deletion apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationSearchCriteria.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
public record OptimizationSearchCriteria(
String name,
String datasetName,
UUID datasetId,
@NonNull EntityType entityType,
Boolean datasetDeleted,
Collection<UUID> datasetIds,
Expand Down
16 changes: 13 additions & 3 deletions apps/opik-backend/src/main/java/com/comet/opik/domain/OptimizationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,24 @@ private OptimizationSearchCriteria resolveDatasetNameFilter(
return searchCriteria;
}

var datasetIds = datasetService.findIdsByPartialName(workspaceId, searchCriteria.datasetName());
var resolvedIds = datasetService.findIdsByPartialName(workspaceId, searchCriteria.datasetName());

if (datasetIds.isEmpty()) {
if (resolvedIds.isEmpty()) {
return null;
}

// If an explicit dataset_id filter was already set, intersect with name-resolved IDs
var existingIds = searchCriteria.datasetIds();
if (CollectionUtils.isNotEmpty(existingIds)) {
var intersection = CollectionUtils.intersection(resolvedIds, existingIds).stream().toList();
if (intersection.isEmpty()) {
return null;
}
resolvedIds = intersection;
}

return searchCriteria.toBuilder()
.datasetIds(datasetIds)
.datasetIds(resolvedIds)
.build();
}

Expand Down
4 changes: 1 addition & 3 deletions .../java/com/comet/opik/domain/experiments/aggregations/AggregationBranchCountsCriteria.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@
@Builder(toBuilder = true)
public record AggregationBranchCountsCriteria(
Set<UUID> experimentIds,
UUID datasetId,
UUID id,
Set<UUID> idsList) {
UUID datasetId) {

public static AggregationBranchCountsCriteria empty() {
return AggregationBranchCountsCriteria.builder().build();
Expand Down
Loading
Loading