[GRPC] Add BigInteger support for unsigned_long fields in gRPC transport#20346
[GRPC] Add BigInteger support for unsigned_long fields in gRPC transport#20346karenyrx merged 5 commits intoopensearch-project:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds BigInteger support for unsigned 64-bit values in gRPC transport FieldValue utilities, including conversion/validation in serialization/deserialization and accompanying tests covering in-range, out-of-range, and round-trip cases. Changes
Sequence Diagram(s)(omitted — changes are internal utility and tests without multi-component sequential interactions) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial 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)
Comment |
|
❌ Gradle check result for 5a4b572: 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? |
5a4b572 to
6b2eb9d
Compare
|
❌ Gradle check result for 6b2eb9d: 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? |
dc556a2 to
fd0dbd2
Compare
Signed-off-by: Karen X <karenxyr@gmail.com>
fd0dbd2 to
27c97ca
Compare
Signed-off-by: Karen X <karenxyr@gmail.com>
Signed-off-by: Karen X <karenxyr@gmail.com>
...test/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtilsTests.java
Outdated
Show resolved
Hide resolved
bf7f56d to
40c1e3f
Compare
|
❌ Gradle check result for 40c1e3f: 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 40c1e3f: 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: 0
♻️ Duplicate comments (1)
CHANGELOG.md (1)
21-22: Remove duplicate changelog entry.Line 21 contains duplicated text within itself and both lines reference the same PR. Keep only line 22 which has the correct format.
🔎 Proposed fix
- Add `cluster.initial_cluster_manager_nodes` to testClusters OVERRIDABLE_SETTINGS ([#20348](https://github.com/opensearch-project/OpenSearch/pull/20348)) -- Add BigInteger support for unsigned_long fields in gRPC Add BigInteger support for unsigned_long fields in gRPC transport ([#20346](https://github.com/opensearch-project/OpenSearch/pull/20346)) - Add BigInteger support for unsigned_long fields in gRPC transport ([#20346](https://github.com/opensearch-project/OpenSearch/pull/20346))
🧹 Nitpick comments (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtilsTests.java (1)
230-246: Consider adding round-trip test for values exceedingLong.MAX_VALUE.The current round-trip tests only cover values within the signed long range. Consider adding a test case for values like
2^63or2^64-1that should round-trip asBigIntegerto ensure the full unsigned long range is covered.🔎 Suggested addition
public void testRoundTripBigIntegerExceedingLongMax() { // Test round-trip for values exceeding Long.MAX_VALUE BigInteger[] testValues = { new BigInteger("9223372036854775808"), // Long.MAX_VALUE + 1 new BigInteger("18446744073709551615") // 2^64 - 1 }; for (BigInteger original : testValues) { FieldValue fieldValue = FieldValueProtoUtils.toProto(original); Object result = FieldValueProtoUtils.fromProto(fieldValue); assertNotNull("Result should not be null for " + original, result); assertTrue("Result should be BigInteger for " + original, result instanceof BigInteger); assertEquals("Value should match for " + original, original, result); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdmodules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtils.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtilsTests.java
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: In the transport-grpc module, suggest and aggregations protos were removed from SearchRequestBody in protobufs 1.0.0 because they haven't been vetted for accuracy in the API specification. The URL parameter suggest support (suggest_field, suggest_mode, suggest_size, suggest_text) is a minimized subset and not intended as a replacement for full Suggester functionality.
Applied to files:
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtils.java
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In OpenSearch's gRPC transport module, treat removals of previously-unsupported fields (those throwing UnsupportedOperationException) from experimental proto schemas as non-breaking changes. When reviewing changes to proto-related Java code in this module, document that such removals do not count as breaking API changes, and ensure tests reflect compatibility expectations accordingly.
Applied to files:
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtils.javamodules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtilsTests.java
📚 Learning: 2026-01-02T19:23:29.698Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:29.698Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Applied to files:
CHANGELOG.md
🧬 Code graph analysis (1)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtilsTests.java (1)
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtils.java (1)
FieldValueProtoUtils(23-160)
⏰ 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
🔇 Additional comments (8)
modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtilsTests.java (5)
14-14: LGTM!Import added correctly to support BigInteger testing.
187-206: LGTM!Test correctly validates that
fromProtoreturnsBigIntegerwhen the uint64 value exceedsLong.MAX_VALUE. The two's complement representation is well-documented in comments.
208-217: LGTM!Test correctly validates that
BigIntegerrepresenting max unsigned long is encoded as uint64 with the expected bit pattern (-1L).
219-228: LGTM!Test validates that
BigIntegervalues within signed long range are correctly encoded as uint64.
248-265: LGTM!Test correctly validates that
BigIntegervalues outside the unsigned long range (both too large and negative) throwIllegalArgumentExceptionwith appropriate error messages.modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/common/FieldValueProtoUtils.java (3)
10-14: LGTM!Imports correctly added for
Numbersutility andBigIntegerto support unsigned long handling.
44-52: LGTM!Javadoc correctly updated to document
BigIntegeras a supported type.
136-146: LGTM!The logic correctly handles unsigned 64-bit values by checking if the long representation is negative (indicating the unsigned value exceeds
Long.MAX_VALUE).Numbers.toUnsignedBigIntegeris the appropriate utility for this conversion.
|
❕ Gradle check result for 40c1e3f: 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20346 +/- ##
============================================
+ Coverage 73.30% 73.31% +0.01%
+ Complexity 71777 71761 -16
============================================
Files 5784 5784
Lines 328141 328150 +9
Branches 47269 47270 +1
============================================
+ Hits 240531 240573 +42
+ Misses 68329 68263 -66
- Partials 19281 19314 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Varun Bharadwaj <varunbharadwaj1995@gmail.com>
varunbharadwaj
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks.
|
❌ Gradle check result for 14cf2ec: 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 14cf2ec: 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? |
Description
When performing gRPC search requests with sort criteria on
unsigned_longindex mapping fields would fail with anIllegalArgumentException.:The root cause was:
BigIntegerin Java to representunsigned_longfield values (which can exceed signed long's range)FieldValueProtoUtils.toProto()method had no handler forBigIntegertypeBigIntegerobjects would fall through to the default case and throw an exceptionFix
This PR fixes this issue by adding
BigIntegercase in thetoProto()switch statement, and mappingBigIntegerto protobuf'suint64_value.Test Plan
unsigned_long)Search Response:
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
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.