FIX: add support for offset and limit on listing aggregations#25943
FIX: add support for offset and limit on listing aggregations#25943
Conversation
|
The Java checkstyle failed. Please run You can install the pre-commit hooks with |
| aggregationsMap.removeIf( | ||
| aggregationMap -> aggregationMap.get("aggType").contains("bucket_selector")); | ||
| for (int j = 0; j < aggregationsMap.size(); j++) { |
There was a problem hiding this comment.
💡 Bug: bucket_sort not excluded from getAggregationMetadata()
The getAggregationMetadata() method (line 159-161) removes bucket_selector from metadata but does not remove bucket_sort. Since bucket_sort is also a pipeline aggregation (not a dimension or metric), it should be excluded just like bucket_selector.
When bucket_sort appears as a leaf node in metadata, it will:
- Add
nullto themetricslist (line 171, since it has nofieldkey) - Add
bucket_sort#paginationto thekeyslist (line 184)
This could corrupt report metadata if the aggregation tree is processed through the generic aggregation path. Currently this doesn't affect listLatestFromSearch (which doesn't use getAggregationMetadata), but it's a latent bug if the tree structure is reused elsewhere.
Suggested fix:
aggregationsMap.removeIf(
aggregationMap -> aggregationMap.get("aggType").contains("bucket_selector")
|| aggregationMap.get("aggType").contains("bucket_sort"));
Was this helpful? React with 👍 / 👎
| // remove bucket_selector from metadata as it is a filter and neither a dimension nor a metric | ||
| aggregationsMap.removeIf( | ||
| aggregationMap -> aggregationMap.get("aggType").contains("bucket_selector")); |
There was a problem hiding this comment.
💡 Edge Case: stats_bucket also not excluded from getAggregationMetadata()
The existing finding notes that bucket_sort isn't excluded from getAggregationMetadata(). The same issue applies to stats_bucket, another pipeline aggregation added in this PR.
At line 159, only bucket_selector is removed:
aggregationsMap.removeIf(
aggregationMap -> aggregationMap.get("aggType").contains("bucket_selector"));If the pagination aggregation tree is ever passed through the data quality report path, both bucket_sort and stats_bucket nodes would leak into the metadata, potentially adding null to the metrics list (since neither has a "field" key).
While this isn't currently triggered (the pagination aggregation doesn't go through getAggregationMetadata()), it's a defensive fix worth making alongside the existing finding. Consider generalizing the exclusion to all pipeline aggregations.
Suggested fix:
// remove pipeline aggregations from metadata as they are neither a dimension nor a metric
aggregationsMap.removeIf(
aggregationMap -> {
String aggType = aggregationMap.get("aggType");
return aggType.contains("bucket_selector")
|| aggType.contains("bucket_sort")
|| aggType.contains("stats_bucket");
});
Was this helpful? React with 👍 / 👎
🔍 CI failure analysis for 6c8f33d: maven-collate-ci failed again (5th consecutive occurrence) due to external Collate workflow failure. Cascading from flaky tests. Pagination code remains correct.Issue
Root CauseCascading failure from external Collate workflow encountering flaky integration tests. Failed Job (Current Run - Commit 6c8f33d)maven-collate-ci (64137552240): External workflow failure DetailsJob Flow:
Pattern Analysismaven-collate-ci failures - all 5 consecutive occurrences:
Conclusion: Persistent pattern of external Collate workflow failures across all CI runs for this PR. Why This Is Unrelated to PR
ConclusionFifth consecutive maven-collate-ci cascading failure. Environmental flakiness in external Collate workflow. Pagination functionality working correctly. The pagination functionality is working correctly and the PR is ready from a code perspective. Code Review 👍 Approved with suggestions 4 resolved / 6 findingsWell-implemented pagination feature with solid test coverage. The dual-aggregation strategy for accurate total counts is sound. One prior finding about pipeline aggregation exclusion in metadata remains unresolved, and 💡 Edge Case:
|
| Auto-apply | Compact |
|
|
Was this helpful? React with 👍 / 👎 | Gitar
|



Describe your changes:
Fixes
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
ListParamsSDK model withoffsetandlatestboolean fields enabling pagination parameters\n - EnhancedEntityTimeSeriesRepository.listLatestFromSearch()to accept limit/offset/sortField/sortType for paginated aggregations\n\n- Implemented dual-aggregation pagination strategy:\n - Addedbucket_sortandstats_bucketpipeline aggregations (Elasticsearch and OpenSearch) to slice result buckets and count totals accurately\n - Parallel aggregation trees when paginating:byTermsfor sliced results +byTermsCountfor exact filtered count\n\n- Updated resource layer to support pagination:\n -TestCaseResolutionStatusResourceandTestCaseResultResourcenow pass pagination parameters to aggregation method\n - Removed documentation stating "offset and limit are ignored"\n\n- Added comprehensive test coverage:\n - Integration test (IncidentPaginationIT) verifies pagination across multiple pages and edge cases (11 test cases, 5-item pages)\n - Unit tests (EntityTimeSeriesRepositoryPaginationTestandSearchAggregationTest) validate aggregation node building and bucket_sort/stats_bucket creation"This will update automatically on new commits.