Skip to content

[OPIK-8023] Add page-boundary coverage for dataset-item JSON-key sorting #7939

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

Open
thiagohora wants to merge 1 commit into
base: main
Choose a base branch
from
+9 −0
Open

[OPIK-8023] Add page-boundary coverage for dataset-item JSON-key sorting #7939

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions ...ackend/src/test/java/com/comet/opik/api/resources/v1/priv/DatasetVersionResourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3591,6 +3591,15 @@ void sortByJsonKeyThroughPushTopLimit(String namespace, String jsonKey, Directio
assertThat(sorted.content())
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactlyElementsOf(expected);

// Page boundary: with size=2, page 2 returns only the trailing item in sort order, exercising the
// push-top-limit OFFSET :top_offset + outer LIMIT path; total stays at the full matching count.
var pageTwo = datasetResourceClient.getDatasetItemsWithExperimentItems(
datasetId, List.of(experimentId), null, null, sorting, 2, 2, API_KEY, TEST_WORKSPACE);
assertThat(pageTwo.total()).isEqualTo(count);
assertThat(pageTwo.content())
.usingRecursiveFieldByFieldElementComparatorIgnoringFields(IGNORED_FIELDS_DATA_ITEM)
.containsExactly(expected.get(count - 1));
Comment on lines +3595 to +3602

@baz-reviewer baz-reviewer Bot Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Page test does not cover push-top-limit

sortByJsonKeyThroughPushTopLimit never finishes the experiment into COMPLETED/CANCELLED nor awaits experiment_item_aggregates, so getAggregationBranchCounts sees hasAggregated=false, hasRaw=true and applyPushTopLimit binds ordinary offset instead of top_dataset_items ... LIMIT :top_limit OFFSET :top_offset — the test never actually exercises the push-top-limit path it targets. Should we finish the experiment and await aggregate materialization, or assert the branch state before issuing the page-two request?

Severity

Want Baz to fix this for you? Activate Fixer

Other fix methods

Fix in Cursor

Prompt for AI Agents 
Before applying, verify this suggestion against the current code. In
`apps/opik-backend/src/test/java/com/comet/opik/api/resources/v1/priv/DatasetVersionResourceTest.java`
around lines 3595-3602, update `sortByJsonKeyThroughPushTopLimit` so the fixture
deterministically selects the push-top-limit path. Finish the experiment with the
appropriate completed or cancelled status, then wait for aggregate materialization (or
explicitly assert the aggregate branch state) before issuing the page-two request,
ensuring the test actually exercises `top_dataset_items ... LIMIT :top_limit OFFSET
:top_offset`.

}

@Test
Expand Down
Loading