Fix numeric type preservation in ML inference query template substitution

[dhrubo-os]

Description

This PR fixes multiple CI test failures caused by recent infrastructure changes.

---

Fix 1: ML inference range query rewrite integration test

Root Cause: The Bedrock connector was configured with pre_process_function and post_process_function. The post-process function transforms the raw Bedrock response into a ModelTensor with dataAsMap=null, making embedding.length() unevaluable via JsonPath. The entire embedding array was being substituted into the range query instead of its length.

Fix (aligned with PR #4657):

• Remove pre_process_function and post_process_function from the Bedrock connector in the test so the raw response is available as dataAsMap
• Use embedding.length() directly to get the embedding dimension as an integer
• Use diary_embedding_size_int (integer field) in the range query

No changes to MLInferenceSearchRequestProcessor — StringSubstitutor works correctly as-is.

---

Fix 2: Cohere connector model update

Root Cause: command-a-03-2025 only supports the Cohere v2 Chat API but the connector uses the v1 API format, causing NO_VALID_RESPONSE_GENERATED errors. command-r was removed September 2025.

Fix: Updated COHERE_CONNECTOR_BLUEPRINT model to command-r-08-2024 which supports the v1 Chat API.

---

Fix 3: bc-fips on unit test classpath in FIPS mode

Root Cause: In FIPS mode, bc-fips is excluded from all Gradle configurations to prevent jar hell when the plugin is installed alongside OpenSearch core. Unit tests don't run inside OpenSearch, so bc-fips is missing at test runtime, causing NoClassDefFoundError in MLSyncUpCronTests.

Fix: Added bc-fips to the unit test task classpath via a detached configuration (bypasses configurations.all exclusion without affecting the plugin bundle).

---

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

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.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The changes replace StringSubstitutor with a manual substitution loop in the ML inference processor to preserve numeric and boolean JSON types as unquoted values during query template substitution. Corresponding tests are added to verify numeric type preservation and an integration test field reference is updated.

Changes

Cohort / File(s)	Summary
Implementation Change plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessor.java	Replaces StringSubstitutor with manual substitution loop that iterates over placeholder-value pairs, inserting numeric and boolean values without quotes while maintaining previous behavior for string values. Removes StringSubstitutor import.
Test Coverage plugin/src/test/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessorTests.java, plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchRequestProcessorIT.java	Adds new test method testExecute_numericTypePreservationInRangeQuery() to verify numeric values remain unquoted in generated range queries. Updates integration test field reference from diary_embedding_size to diary_embedding_size_int in query template.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing numeric type preservation in ML inference query template substitution, which is the core functional improvement across all modified files.
Description check	✅ Passed	The PR description is well-structured with clear sections describing the root causes and fixes for three CI test failures, and follows most of the template requirements.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessor.java (1)

385-399: Misleading comment and redundant if-else logic.

The comment on line 394 states "Use StringSubstitutor for non-numeric values" but StringSubstitutor is not used anywhere in this code block. Additionally, both branches of the if-else execute identical code (String.valueOf(entry.getValue())), making the conditional redundant.

♻️ Suggested simplification with accurate comment

 // Manually substitute to preserve numeric types in JSON
 String result = queryTemplate;
 for (Map.Entry<String, Object> entry : valuesMap.entrySet()) {
     String placeholder = "${" + entry.getKey() + "}";
-    String replacement;
-    if (entry.getValue() instanceof Number || entry.getValue() instanceof Boolean) {
-        // Numbers and booleans should not be quoted in JSON
-        replacement = String.valueOf(entry.getValue());
-    } else {
-        // Use StringSubstitutor for non-numeric values to maintain existing behavior
-        replacement = String.valueOf(entry.getValue());
-    }
+    // String.valueOf preserves numeric/boolean representation without quotes,
+    // allowing proper JSON type handling in templates
+    String replacement = String.valueOf(entry.getValue());
     result = result.replace(placeholder, replacement);
 }
 return result;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessor.java`
around lines 385 - 399, The current substitution loop in
MLInferenceSearchRequestProcessor contains a misleading comment about
StringSubstitutor and a redundant if-else that always calls String.valueOf;
simplify by removing the conditional and updating the comment: iterate
valuesMap, form placeholder = "${" + entry.getKey() + "}", set replacement =
String.valueOf(entry.getValue()) for all types (this preserves numeric/boolean
literal string forms), and replace the placeholder in queryTemplate; update the
comment to accurately describe the manual substitution preserving JSON literal
forms.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8bb78f6 and 139b568.

📒 Files selected for processing (3)

• plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessor.java
• plugin/src/test/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessorTests.java
• plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchRequestProcessorIT.java

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2025-12-18T16:02:36.322Z

Learnt from: akolarkunnu
Repo: opensearch-project/ml-commons PR: 3919
File: plugin/src/test/java/org/opensearch/ml/cluster/MLSyncUpCronTests.java:184-191
Timestamp: 2025-12-18T16:02:36.322Z
Learning: In tests that exercise initialization logic (e.g., initMLConfig), verify idempotence by calling the init method twice and asserting that the master key is initialized once and cached for subsequent calls. The test should confirm that repeated initializations yield the same result and no unnecessary recomputation. Apply this pattern to similar test files under plugin/src/test/java/org/opensearch/ml/cluster and other modules that initialize shared state.

Applied to files:

• plugin/src/test/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessorTests.java
• plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchRequestProcessorIT.java

⏰ 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). (4)

• GitHub Check: Build and Test MLCommons Plugin on linux (25)
• GitHub Check: Build and Test MLCommons Plugin on linux (21)
• GitHub Check: Build and Test MLCommons Plugin on Windows (21)
• GitHub Check: Build and Test MLCommons Plugin on Windows (25)

🔇 Additional comments (2)

plugin/src/test/java/org/opensearch/ml/rest/RestMLInferenceSearchRequestProcessorIT.java (1)

346-346: LGTM!

The change correctly switches the range query target from diary_embedding_size (keyword field) to diary_embedding_size_int (integer field), aligning with the stricter numeric validation in OpenSearch. The index mapping and test documents already support both fields.

plugin/src/test/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessorTests.java (1)

546-606: LGTM!

This test thoroughly validates the numeric type preservation fix. It correctly:

• Uses a range query template with an unquoted ${modelPrediction} placeholder
• Simulates an embedding array of 1536 elements to verify embedding.length() returns an Integer
• Asserts the generated RangeQuery uses the numeric value 1536 with proper bounds (to(1536), includeUpper(true))

The test aligns well with the PR objective of ensuring JsonPath expressions returning Integer values are substituted as unquoted numbers in range queries.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@plugin/src/main/java/org/opensearch/ml/processor/MLInferenceSearchRequestProcessor.java`:
- Around line 385-399: The current substitution loop in
MLInferenceSearchRequestProcessor contains a misleading comment about
StringSubstitutor and a redundant if-else that always calls String.valueOf;
simplify by removing the conditional and updating the comment: iterate
valuesMap, form placeholder = "${" + entry.getKey() + "}", set replacement =
String.valueOf(entry.getValue()) for all types (this preserves numeric/boolean
literal string forms), and replace the placeholder in queryTemplate; update the
comment to accurately describe the manual substitution preserving JSON literal
forms.

[dhrubo-os]

Updating PR description to reflect all changes made.

[codecov]

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.25%. Comparing base (f78efd5) to head (fb4ccc1).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...l/processor/MLInferenceSearchRequestProcessor.java	84.61%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #4656   +/-   ##
=========================================
  Coverage     77.25%   77.25%           
  Complexity    11248    11248           
=========================================
  Files           944      944           
  Lines         50460    50471   +11     
  Branches       6073     6076    +3     
=========================================
+ Hits          38984    38993    +9     
- Misses         8921     8922    +1     
- Partials       2555     2556    +1     

Flag	Coverage Δ
ml-commons	77.25% <84.61%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[akolarkunnu]

elseif and else body looks the same here.

else if (entry.getValue() instanceof Boolean) {
        replacement = String.valueOf(entry.getValue());
 } else {
          replacement = String.valueOf(entry.getValue());
}

can be replaced with

else {
          replacement = String.valueOf(entry.getValue());
}

[akolarkunnu]

Listener -> listener

[mingshl]

LGTM, Thanks @dhrubo-os for cherry pick the commit from #4657 to unblock the CI.

I will close the separate PR.