Aggregation Array OOB Error #20204
Aggregation Array OOB Error #20204
Conversation
Signed-off-by: Anthony Leong <[email protected]>
WalkthroughRefactors Ranges from static to instance methods, updates callers, adds an integration test reproducing a date-histogram + unsigned_long range aggregation scenario, and updates release notes to document the fix for an array-out-of-bounds during aggregation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20204 +/- ##
============================================
- Coverage 73.29% 73.26% -0.03%
+ Complexity 71780 71757 -23
============================================
Files 5795 5795
Lines 328297 328297
Branches 47282 47282
============================================
- Hits 240612 240519 -93
- Misses 68368 68464 +96
+ Partials 19317 19314 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Nice catch @ajleong623 - thanks for finding & fixing it. Can you add up a quick test case as well to verify the same - before and after your change! |
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Anthony Leong <[email protected]>
|
❌ Gradle check result for 7895552: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Anthony Leong <[email protected]>
|
❌ Gradle check result for 3b8467c: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
@ajleong623 Gentle nudge that we could make this bug fix to 3.4 if we can merge it this week. |
Signed-off-by: Anthony Leong <[email protected]>
|
❌ Gradle check result for a2090dd: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Anthony Leong <[email protected]>
ajleong623
requested review from
Bukhtawar,
CEHENKLE,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2 and
msfroh
as code owners
Signed-off-by: Anthony Leong <[email protected]>
Signed-off-by: Sandesh Kumar <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
release-notes/opensearch.release-notes-3.4.0.md (1)
11-11: Apply hyphenation to compound adjectives.Lines 11 and 13 contain compound adjectives that should be hyphenated when modifying nouns: "context aware" → "context-aware", "Server side" → "server-side", "client side" → "client-side".
Apply these diffs:
- Add a mapper for context aware segments grouping criteria ([#19233](https://github.com/opensearch-project/OpenSearch/pull/19233)) + Add a mapper for context-aware segments grouping criteria ([#19233](https://github.com/opensearch-project/OpenSearch/pull/19233))- Add support for repository with Server side encryption enabled and client side encryption as well based on a flag ([#19630](https://github.com/opensearch-project/OpenSearch/pull/19630)) + Add support for repository with server-side encryption enabled and client-side encryption as well based on a flag ([#19630](https://github.com/opensearch-project/OpenSearch/pull/19630))Also applies to: 13-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
release-notes/opensearch.release-notes-3.4.0.md(5 hunks)
🧰 Additional context used
🪛 LanguageTool
release-notes/opensearch.release-notes-3.4.0.md
[uncategorized] ~11-~11: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nSearch/pull/19539)) - Add a mapper for context aware segments grouping criteria ([#19233](ht...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~13-~13: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...568)) - Add support for repository with Server side encryption enabled and client side encr...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~13-~13: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...with Server side encryption enabled and client side encryption as well based on a flag ([#1...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: assemble (21, macos-15)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: verify-changelog
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: detect-breaking-change
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
🔇 Additional comments (1)
release-notes/opensearch.release-notes-3.4.0.md (1)
105-105: New release notes entry correctly documents the fix.The addition of the new entry "Fix array out of bounds during aggregation" at line 105 appropriately captures the bug fix from PR #20204 and is well-placed within the Fixed section.
Signed-off-by: Sandesh Kumar <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
release-notes/opensearch.release-notes-3.4.0.md (2)
9-9: Typo corrected: "unsigned_long" spelling fixed.The typo "usigned_long" has been corrected to "unsigned_long", aligning with the correct field type name referenced in the PR.
88-88: Formatting corrected: space added before PR reference.The spacing has been standardized to match other entries in the release notes.
🧹 Nitpick comments (1)
release-notes/opensearch.release-notes-3.4.0.md (1)
11-11: Add hyphens to compound adjectives per style guidelines.Per static analysis, compound adjectives modifying nouns should use hyphens for clarity:
- Line 11: "context aware segments" → "context-aware segments"
- Line 13: "Server side encryption" → "Server-side encryption" (two instances)
Apply this diff:
- Add a mapper for context aware segments grouping criteria ([#19233](https://github.com/opensearch-project/OpenSearch/pull/19233)) + Add a mapper for context-aware segments grouping criteria ([#19233](https://github.com/opensearch-project/OpenSearch/pull/19233))- Add support for repository with Server side encryption enabled and client side encryption as well based on a flag ([#19630](https://github.com/opensearch-project/OpenSearch/pull/19630)) + Add support for repository with Server-side encryption enabled and client-side encryption as well based on a flag ([#19630](https://github.com/opensearch-project/OpenSearch/pull/19630))Also applies to: 13-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
release-notes/opensearch.release-notes-3.4.0.md(5 hunks)
🧰 Additional context used
🪛 LanguageTool
release-notes/opensearch.release-notes-3.4.0.md
[uncategorized] ~11-~11: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...nSearch/pull/19539)) - Add a mapper for context aware segments grouping criteria ([#19233](ht...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~13-~13: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...568)) - Add support for repository with Server side encryption enabled and client side encr...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~13-~13: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...with Server side encryption enabled and client side encryption as well based on a flag ([#1...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
🔇 Additional comments (1)
release-notes/opensearch.release-notes-3.4.0.md (1)
105-105: New entry for array out of bounds fix added appropriately.Line 105 correctly documents the fix for the array out of bounds exception during aggregations, referencing PR #20204. The formatting aligns with other entries, and the placement in the "Fixed" section is appropriate.
|
❌ Gradle check result for d90d6a9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
|
LGTM! Good find @ajleong623. Since this was introduced in 3.0, so just porting to 3.4 is good enough: #14464 |
|
Flaky test failure: |
|
❌ Gradle check result for d90d6a9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
sandeshkr419
merged commit 947efd3
into
opensearch-project:main
sandeshkr419
added
backport 3.4
prudhvigodithi
added
backport 3.4
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-3.4 3.4
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-3.4
# Create a new branch
git switch --create backport/backport-20204-to-3.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 947efd33b930c570055eaf5ace272fe092b2bb5f
# Push it to GitHub
git push --set-upstream origin backport/backport-20204-to-3.4
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-3.4Then, create a pull request where the |
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-3.4 3.4
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-3.4
# Create a new branch
git switch --create backport/backport-20204-to-3.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 947efd33b930c570055eaf5ace272fe092b2bb5f
# Push it to GitHub
git push --set-upstream origin backport/backport-20204-to-3.4
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-3.4Then, create a pull request where the |
1 similar comment
|
The backport to To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-3.4 3.4
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-3.4
# Create a new branch
git switch --create backport/backport-20204-to-3.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 947efd33b930c570055eaf5ace272fe092b2bb5f
# Push it to GitHub
git push --set-upstream origin backport/backport-20204-to-3.4
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-3.4Then, create a pull request where the |
Co-authored-by: Sandesh Kumar <[email protected]>
Co-authored-by: Anthony Leong <[email protected]>
Co-authored-by: Sandesh Kumar <[email protected]>
Co-authored-by: Sandesh Kumar <[email protected]>
4 participants
Description
In Ranges.java, the comparator attribute is static which means that every time a new Ranges method is created, the comparator is overridden. This leads to the OOB error because the wrong array comparator might used for the wrong array length if there are multiple aggregations.
Related Issues
Resolves #19365
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
Bug Fixes
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.