[GRPC] Add support for missing proto fields in FunctionScore and Highlight #20169
Conversation
WalkthroughAdds protobuf-to-builder mappings for highlight field-level settings, propagates MultiValueMode for decay functions (exp/gauss/linear), accepts int64 seeds for random scoring, and adds corresponding unit tests and a small DecayFunction proto utility class. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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). (1)
🔇 Additional comments (2)
Comment |
…light Signed-off-by: Karen X <[email protected]>
karenyrx
force-pushed
the
funcscorehighlight
branch
from
4d32f49 to
fd08ef7
Compare
|
❌ Gradle check result for fd08ef7: 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? |
| assertEquals("1", builder.termsLookup().id()); | ||
| assertEquals("tags", builder.termsLookup().path()); | ||
| assertEquals("r", builder.termsLookup().routing()); | ||
| assertTrue("store should be true", builder.termsLookup().store()); |
There was a problem hiding this comment.
Adding a test here for the field that was added in https://github.com/opensearch-project/OpenSearch/pull/20162/files#diff-6089ea05ecb3ee05e04ecf077a33720e435dcd0fa54075d5d632a84acde02e83
(This change was meant to go into the previous PR but rebase error caused it to be dropped)
Signed-off-by: Karen X <[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 (3)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/ExpDecayFunctionProtoUtilsTests.java (1)
182-257: Solid multi_value_mode coverage; consider DRYing test setup and making default behavior more explicitThe four new tests nicely exercise
fromProtoforMULTI_VALUE_MODE_MIN,MAX,AVG, andUNSPECIFIED, and the assertions on both builder type and field name look correct.Two minor, optional improvements:
- The setup for
NumericDecayPlacement+DecayPlacement+DecayFunctionis duplicated across these tests. A small helper (e.g.,buildNumericDecayFunction(String field, double origin, double scale, MultiValueMode mode)) would make the tests shorter and easier to extend if new modes are added later.- The UNSPECIFIED test encodes the assumption that the builder’s default
MultiValueModeisMIN. Right nowExpDecayFunctionProtoUtils.fromProtorelies on that default rather than settingMINexplicitly when the proto mode is UNSPECIFIED. If the underlyingExponentialDecayFunctionBuilderdefault ever changes, behavior would silently shift. Consider updatingfromPrototo explicitly setMultiValueMode.MINfor UNSPECIFIED and keep this test asserting that explicit behavior instead of the builder default.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/RandomScoreFunctionProtoUtilsTests.java (1)
34-47: Good coverage for the new int64 seed pathThis test mirrors the existing int32/string cases and correctly verifies both field propagation and the expected
Long.hashCode(...)-based seed value, so it should catch regressions in the long-seed handling. If you later expand this area, you might optionally add a boundary/negative int64 seed case, but it’s not required for this change set.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtilsTests.java (1)
449-462: Consider verifying the applied pre/post tags.The test verifies the field was created but doesn't assert that the styled tags were actually applied. Since
applyTagsSchemaToFieldsets specific pre/post tags, consider verifying them:// The tags schema should have been applied - verify the field was created HighlightBuilder.Field field = fields.get(0); assertEquals("title", field.name()); + + // Verify styled tags were applied + String[] preTags = field.preTags(); + assertNotNull(preTags); + assertArrayEquals(HighlightBuilder.DEFAULT_STYLED_PRE_TAG, preTags); + + String[] postTags = field.postTags(); + assertNotNull(postTags); + assertArrayEquals(HighlightBuilder.DEFAULT_STYLED_POST_TAGS, postTags); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtils.java(3 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/DecayFunctionProtoUtils.java(1 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/ExpDecayFunctionProtoUtils.java(4 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/GaussDecayFunctionProtoUtils.java(4 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/LinearDecayFunctionProtoUtils.java(5 hunks)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/RandomScoreFunctionProtoUtils.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtilsTests.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtilsTests.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/ExpDecayFunctionProtoUtilsTests.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/GaussDecayFunctionProtoUtilsTests.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/LinearDecayFunctionProtoUtilsTests.java(1 hunks)modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/RandomScoreFunctionProtoUtilsTests.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/GaussDecayFunctionProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/DecayFunctionProtoUtils.java (1)
DecayFunctionProtoUtils(17-38)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/ExpDecayFunctionProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/DecayFunctionProtoUtils.java (1)
DecayFunctionProtoUtils(17-38)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/LinearDecayFunctionProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/LinearDecayFunctionProtoUtils.java (1)
LinearDecayFunctionProtoUtils(28-168)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtils.java (1)
HighlightBuilderProtoUtils(29-320)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtilsTests.java (2)
server/src/main/java/org/opensearch/indices/TermsLookup.java (1)
TermsLookup(60-269)modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtils.java (1)
TermsQueryBuilderProtoUtils(28-257)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/LinearDecayFunctionProtoUtils.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/DecayFunctionProtoUtils.java (1)
DecayFunctionProtoUtils(17-38)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/ExpDecayFunctionProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/ExpDecayFunctionProtoUtils.java (1)
ExpDecayFunctionProtoUtils(28-150)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/RandomScoreFunctionProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/RandomScoreFunctionProtoUtils.java (1)
RandomScoreFunctionProtoUtils(19-60)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/GaussDecayFunctionProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/GaussDecayFunctionProtoUtils.java (1)
GaussDecayFunctionProtoUtils(28-150)
⏰ 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). (19)
- GitHub Check: gradle-check
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, windows-latest)
🔇 Additional comments (14)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/RandomScoreFunctionProtoUtils.java (1)
51-52: Int64 seed handling looks correct and consistentAdding the
hasInt64()branch and passingseed.getInt64()intoRandomScoreFunctionBuilder.seed(...)cleanly extends existing int32/string support without changing prior behavior. The precedence order (int32 → int64 → string) is reasonable given the oneof semantics of the proto.CHANGELOG.md (1)
76-77: LGTM!The changelog entry is correctly formatted and placed in the appropriate "Changed" section, following the existing conventions.
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtils.java (3)
155-157: LGTM!The TODO comment is well-documented and clearly indicates the dependency on external spec fixes and protobuf version upgrade.
258-272: LGTM!The field-level settings implementation correctly mirrors the top-level handling patterns for
order,phraseLimit, andrequireFieldMatch. ThetagsSchemaappropriately uses a helper method sinceHighlightBuilder.Fielddoesn't expose a directtagsSchema()setter like the parentHighlightBuilderdoes.
309-319: LGTM!The helper method correctly applies the styled tags schema using OpenSearch's built-in constants. The TODO for
HIGHLIGHTER_TAGS_SCHEMA_DEFAULTis appropriately tracked pending the spec fix. The defensiveIllegalArgumentExceptionin the default case is a reasonable approach for handling unexpected enum values.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/HighlightBuilderProtoUtilsTests.java (2)
478-536: LGTM!Comprehensive test covering the combination of all field settings. Good practice to document the package-private fields that can't be verified directly.
407-419: LGTM!The new field-level tests for
order,phraseLimit,requireFieldMatch, andforceSourcecorrectly validate the proto-to-builder mappings and follow the existing test patterns in the file.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtilsTests.java (1)
175-180: LGTM! Enhanced test coverage for lookup property propagation.The added assertions comprehensively validate that all lookup properties (index, id, path, routing, store) are correctly propagated from the protobuf to the builder.
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/LinearDecayFunctionProtoUtilsTests.java (1)
196-271: LGTM! Well-structured tests for MultiValueMode handling.The four new tests comprehensively cover the MultiValueMode enum values (MIN, MAX, AVG, UNSPECIFIED) and follow consistent patterns with clear assertions. The test structure aligns well with existing tests in the file. The tests are well-written with appropriate setup, execution, and verification phases. If
MultiValueMode.MINis not the actual default forLinearDecayFunctionBuilder, thetestFromProtoWithMultiValueModeUnspecifiedtest will catch this at runtime.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/ExpDecayFunctionProtoUtils.java (1)
44-70: LGTM: Multi-value mode integration follows a clean pattern.The refactored
fromProtomethod correctly:
- Uses a concrete
ExponentialDecayFunctionBuildervariable to enable post-processing- Applies
MultiValueModeonly when present and notUNSPECIFIED- Delegates to the shared
DecayFunctionProtoUtils.parseMultiValueModefor consistent mappingThe pattern is consistent with sibling implementations (
GaussDecayFunctionProtoUtils,LinearDecayFunctionProtoUtils).modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/LinearDecayFunctionProtoUtils.java (1)
44-70: LGTM: Implementation is consistent with sibling decay function utilities.The multi-value mode handling pattern is correctly applied, matching
ExpDecayFunctionProtoUtilsandGaussDecayFunctionProtoUtils.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/GaussDecayFunctionProtoUtils.java (1)
44-70: LGTM: Consistent implementation across decay function utilities.The multi-value mode handling is correctly implemented and consistent with
ExpDecayFunctionProtoUtilsandLinearDecayFunctionProtoUtils.modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/query/functionscore/GaussDecayFunctionProtoUtilsTests.java (2)
195-251: Good test coverage for MIN, MAX, and AVG multi-value modes.The tests correctly verify that proto
MultiValueModevalues are properly converted and applied to theGaussDecayFunctionBuilder.
252-270: Verify the default MultiValueMode behavior is intentional and stable.The test asserts that
MULTI_VALUE_MODE_UNSPECIFIEDdefaults toMultiValueMode.MIN(line 269). This assumption relies on implicit behavior inGaussDecayFunctionBuilder. Add a comment explaining where this default originates (e.g., "Defaults to MIN per DecayFunction specification" or reference the relevant builder/proto documentation), or extract the expected default to a constant to make the assumption explicit and prevent unexpected breakage if the default changes.
...nsearch/transport/grpc/proto/request/search/query/functionscore/DecayFunctionProtoUtils.java
Show resolved
Hide resolved
...g/opensearch/transport/grpc/proto/request/search/query/TermsQueryBuilderProtoUtilsTests.java
Show resolved
Hide resolved
Signed-off-by: Karen X <[email protected]>
karenyrx
force-pushed
the
funcscorehighlight
branch
from
2079347 to
05123eb
Compare
|
❌ Gradle check result for 05123eb: 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? |
2 participants
Description
Add support for some missing fields in function-score and highlight related fields in GRPC Search API.
Note: The 2 TODOs in Highlight-related proto utils code will be made once spec fixes in https://github.com/opensearch-project/opensearch-api-specification/pull/1003/files is merged, and a new protobuf version is published.
Related Issues
#19526
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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.