[OPIK-8023] Add page-boundary coverage for dataset-item JSON-key sorting - #7939
Open
thiagohora wants to merge 1 commit into
Open
[OPIK-8023] Add page-boundary coverage for dataset-item JSON-key sorting#7939thiagohora wants to merge 1 commit into
thiagohora wants to merge 1 commit into
Conversation
Follow-up to #7935. Extend the parameterized version-DAO sort test with a page=2,size=2 request per namespace/direction, asserting the single trailing item on the boundary page and that total stays at the full matching count. This exercises the push-top-limit OFFSET :top_offset + outer LIMIT path, previously covered only at page=1,size=10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
github-actions
Bot
added
java
Contributor
⏱️ pre-commit per-hook timing
⏭️ 42 skipped (no matching files changed)
|
Collaborator
|
No test needed here. This PR is test-only: the whole diff is 9 added lines in DatasetVersionResourceTest.java asserting page 2 of a size-2 page over JSON-key sorting. No production code changes, so there is no new behaviour for an e2e test to cover — the assertion you added is itself the coverage for the top_offset/LIMIT path. Advisory, from the QA test radar. Nothing here blocks this PR, and anything it proposes is a draft for review. |
Contributor
Backend Tests - Integration Group 12224 tests 223 ✅ 4m 1s ⏱️ For more details on these failures, see this check. Results for commit 50cd9f9. ♻️ This comment has been updated with latest results. |
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
Comment on lines
+3595
to
+3602
| // 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)); |
Contributor
There was a problem hiding this comment.
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?
Want Baz to fix this for you? Activate Fixer
Other fix methods
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`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Details
Follow-up to #7935. Extends
DatasetVersionResourceTest#sortByJsonKeyThroughPushTopLimitwith apage=2, size=2request per namespace/direction, asserting the single trailing item on the boundary page and thattotalstays at the full matching count. This exercises the push-top-limitOFFSET :top_offset+ outerLIMITpath, which was previously covered only atpage=1, size=10.Change checklist
Issues
AI-WATERMARK
AI-WATERMARK: yes
Testing
mvn -o test -Dtest='DatasetVersionResourceTest$ExperimentDatasetVersionLinking#sortByJsonKeyThroughPushTopLimit'— 6/6 pass (output/input/metadata x ASC/DESC), each now also asserting page 2 (size 2) returns the single boundary item withtotal=3mvn -o spotless:check— cleanDocumentation
N/A — test-only change.
🤖 Generated with Claude Code