Adding public getter function for source field mapper excludes#20205
Conversation
buddharajusahil
requested review from
a team,
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
github-actions
bot
added
_No response_
enhancement
WalkthroughAdded a public accessor Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (20)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java (1)
285-287: Accessor implementation is correct; consider defensive copy if immutability is desiredThe new
getExcludes()correctly exposes the configured excludes and aligns with the stated PR goal; no functional issues here. The only consideration is that returning the backingString[]allows callers to mutate the array contents and effectively change the mapper’s internal state. If you want to keepSourceFieldMappereffectively immutable, you could instead return a copy or an unmodifiable view:- public String[] getExcludes() { - return excludes; - } + public String[] getExcludes() { + return Arrays.copyOf(excludes, excludes.length); + }This is optional and depends on how much you want to protect against external mutation of mapper configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java(1 hunks)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
buddharajusahil
force-pushed
the
feature/source-excludes-public
branch
from
9100f2c to
0b686af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java (1)
285-287: Avoid leaking internalexcludesarray; return a defensive copy insteadReturning the backing
String[]lets callers mutate internal state of aSourceFieldMapper, which can violate immutability assumptions and cause hard‑to‑trace bugs under concurrent access. Consider returning a copy instead.- public String[] getExcludes() { - return excludes; - } + public String[] getExcludes() { + return excludes.clone(); + }CHANGELOG.md (1)
40-40: Changelog entry correctly documents the new public getterThe new bullet accurately reflects the
SourceFieldMapperAPI change; no functional concerns. If you want to polish wording, consider “…for use with k‑NN derived source paths” for slightly smoother phrasing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java(1 hunks)
⏰ 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). (20)
- GitHub Check: gradle-check
- GitHub Check: Analyze (java)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: detect-breaking-change
server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20205 +/- ##
============================================
- Coverage 72.82% 72.71% -0.12%
+ Complexity 71315 71278 -37
============================================
Files 5795 5795
Lines 328297 328303 +6
Branches 47282 47283 +1
============================================
- Hits 239089 238716 -373
- Misses 69893 70316 +423
+ Partials 19315 19271 -44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
buddharajusahil
force-pushed
the
feature/source-excludes-public
branch
from
0b686af to
a4f4556
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java (1)
285-287: Defensive copy viaclone()is correct; consider documenting contractReturning
excludes.clone()cleanly exposes the configuration without leaking internal mutable state, and keeps the existingString[]-based API surface intact. From a correctness and encapsulation standpoint this looks good.As a minor follow‑up, consider adding brief Javadoc on
getExcludes()clarifying that:
- it never returns
null, and- it returns a copy of the excludes array that callers are free to modify.
That will help future users of this public API understand the method’s guarantees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
⏰ 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). (1)
- GitHub Check: gradle-check
|
❕ Gradle check result for a4f4556: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.md (1)
40-40: Consider adding the specific method name for clarity.The changelog entry correctly documents the new feature in the appropriate section. For better clarity and consistency with documentation practices, consider mentioning the specific method name
getExcludes():- Added public getter method in `SourceFieldMapper` to be used in KNN derived source path ([#20205](https://github.com/opensearch-project/OpenSearch/pull/20205)) + Added public getter method `getExcludes()` in `SourceFieldMapper` to be used in KNN derived source path ([#20205](https://github.com/opensearch-project/OpenSearch/pull/20205))This makes the changelog entry more informative for users reviewing release notes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
⏰ 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). (21)
- GitHub Check: gradle-check
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: detect-breaking-change
- GitHub Check: Mend Security Check
|
❌ Gradle check result for ea08818: 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? |
|
❌ Gradle check result for ea08818: 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? |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~13-~13: Use a hyphen to join words.
Context: .../pull/19539)) - Add a mapper for context aware segments grouping criteria ([#1923...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~15-~15: 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)
[grammar] ~15-~15: Use a hyphen to join words.
Context: ...erver side encryption enabled and client side encryption as well based on a flag....
(QB_NEW_EN_HYPHEN)
[uncategorized] ~32-~32: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...enSearch/pull/19878)) - Add support for context aware segments ([#19098](https://github.com/o...
(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). (21)
- GitHub Check: gradle-check
- GitHub Check: Mend Security Check
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
CHANGELOG.md (1)
40-40: Changelog entry looks accurate; consider consistent naming (“k-NN” / “derived source path(s)”).
buddharajusahil
force-pushed
the
feature/source-excludes-public
branch
2 times, most recently
from
c9b5b31 to
10da324
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
CHANGELOG.md (1)
9-40: Fix the diff to show only your changelog entry.The diff incorrectly shows the entire "Added" section (lines 9-40) as modified when only your new entry on line 41 should be added. This appears to be a merge conflict resolution issue. Please rebase or merge properly so that only line 41 appears in the diff.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(1 hunks)server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~14-~14: Use a hyphen to join words.
Context: .../pull/19539)) - Add a mapper for context aware segments grouping criteria ([#1923...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~16-~16: 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)
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...erver side encryption enabled and client side encryption as well based on a flag....
(QB_NEW_EN_HYPHEN)
[uncategorized] ~33-~33: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...enSearch/pull/19878)) - Add support for context aware segments ([#19098](https://github.com/o...
(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). (20)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (21, windows-latest)
🔇 Additional comments (2)
CHANGELOG.md (1)
41-41: LGTM!The changelog entry accurately describes the addition of the public getter method and follows the established format.
server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java (1)
286-286: Good use of defensive copying.Returning a clone of the array prevents external code from mutating the internal state of the mapper, which is the correct approach for exposing internal collections.
Signed-off-by: Sahil Buddharaju <sahilbud@amazon.com>
buddharajusahil
force-pushed
the
feature/source-excludes-public
branch
from
10da324 to
144077f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.md(2 hunks)server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/src/main/java/org/opensearch/index/mapper/SourceFieldMapper.java
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[grammar] ~9-~9: Use a hyphen to join words.
Context: ...in SourceFieldMapper to be used in KNN derived source path ([#20205](https://gi...
(QB_NEW_EN_HYPHEN)
⏰ 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). (16)
- GitHub Check: gradle-check
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: Analyze (java)
|
❌ Gradle check result for 144077f: null 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? |
buddharajusahil
force-pushed
the
feature/source-excludes-public
branch
from
ac41dfb to
c940b31
Compare
|
❕ Gradle check result for c940b31: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Signed-off-by: Sahil Buddharaju <sahilbud@amazon.com>
buddharajusahil
force-pushed
the
feature/source-excludes-public
branch
from
c940b31 to
72a4289
Compare
|
❕ Gradle check result for 72a4289: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
…earch-project#20205) * Adding public getter function for source field mapper excludes Signed-off-by: Sahil Buddharaju <sahilbud@amazon.com> * Retrigger CI Signed-off-by: Sahil Buddharaju <sahilbud@amazon.com> --------- Signed-off-by: Sahil Buddharaju <sahilbud@amazon.com> Co-authored-by: Sahil Buddharaju <sahilbud@amazon.com>
2 participants
Description
This change is adding a public getter function for the excludes list in SourceFieldMapper.
Related Issues
Resolves #20201
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
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.