Making changes in InternalValueCount and InternalAvg to support Scripted metric in reducing aggregation by tandonks · Pull Request #18288 · opensearch-project/OpenSearch

tandonks · 2025-05-14T03:20:15Z

Description

We had introduced the support for searching rollup and raw indices together in this PR but there was a bug in this due to class cast exception error when trying to reduce InternalValueCount and InternalAggregation as both used InternalScriptedMetric in their aggregation builder here. So this PR fixes this issue by introducing support for InternalScriptedMetric when trying to reduce these aggregations. Also included tests to cover these changes.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

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.

…ted metric in reducing agggregation Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

…e and value count aggregations Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

github-actions · 2025-05-14T03:31:15Z

❌ Gradle check result for 4c363d5: 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: Kshitij Tandon <tandonks@amazon.com>

github-actions · 2025-05-14T04:42:13Z

❌ Gradle check result for 91e807a: 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?

backslasht · 2025-05-27T03:17:20Z

CHANGELOG.md

 - Introduce system generated ingest pipeline ([#17817](https://github.com/opensearch-project/OpenSearch/pull/17817)))
 - Support create mode in pull-based ingestion and add retries for transient failures ([#18250](https://github.com/opensearch-project/OpenSearch/pull/18250)))
 - Decouple the init of Crypto Plugin and KeyProvider in CryptoRegistry ([18270](https://github.com/opensearch-project/OpenSearch/pull18270)))
+- Supporting Scripted Metric Aggregation when reducing aggregations in InternalValueCount and InternalAvg ([18288](https://github.com/opensearch-project/OpenSearch/pull18270)))


Suggested change

- Supporting Scripted Metric Aggregation when reducing aggregations in InternalValueCount and InternalAvg ([18288](https://github.com/opensearch-project/OpenSearch/pull18270)))

- Supporting Scripted Metric Aggregation when reducing aggregations in InternalValueCount and InternalAvg ([18288](https://github.com/opensearch-project/OpenSearch/pull/18288)))

backslasht · 2025-05-27T03:32:53Z

server/src/main/java/org/opensearch/search/aggregations/metrics/InternalAvg.java

+                            "Expected double array with [sum, count] from ScriptedMetric but got array of length " + values.length
+                        );
+                    }
+                    count += (long) values[1];  // count is at index 1


Suggested change

count += (long) values[1]; // count is at index 1

count += Double.valueOf(values[1]).longValue(); // count is at index 1

backslasht · 2025-05-27T03:33:14Z

server/src/main/java/org/opensearch/search/aggregations/metrics/InternalAvg.java

+                    throw new IllegalArgumentException(
+                        "Expected double array[sum, count] from ScriptedMetric but got ["
+                            + (value == null ? "null" : value.getClass().getName())
+                            + "]"


is this user visible error string?

backslasht · 2025-05-27T03:33:24Z

server/src/main/java/org/opensearch/search/aggregations/metrics/InternalAvg.java

+                if (value instanceof double[]) {
+                    double[] values = (double[]) value;
+                    if (values.length != 2) {
+                        throw new IllegalArgumentException(


is this user visible error string?

Yup from what I checked, all these would be user visible. I had included them for faster debugging in the future, but do we not want to expose these logs to the user?

Can the statements be more user friendly? we can look at existing code and take inspiration from it. Can you please check the existing code where IllegalArgumentException is thrown?

backslasht · 2025-05-27T03:37:04Z

server/src/test/java/org/opensearch/search/aggregations/metrics/InternalAvgTests.java

        }
    }

+    public void testReduceWithScriptedMetric() {


can we add test cases for negative cases as well?

when the values is not double[]

when the value is not InternalScriptedMetric

…t#18184) Signed-off-by: Lingxi Chen <lingxich@amazon.com> Co-authored-by: Lingxi Chen <lingxich@amazon.com>

These tests have been disabled for over 4 years. Signed-off-by: Andrew Ross <andrross@amazon.com>

…h-project#18321) Signed-off-by: Andrew Ross <andrross@amazon.com>

Signed-off-by: Andriy Redko <drreta@gmail.com>

@rishabhmaurya

Following the [nomination process][1], I have nominated and other maintainers have agreed to add Rishabh Maurya (@rishabhmaurya) as a co-maintainer of the OpenSearch repository. Rishabh has kindly accepted the invitation. [1]: https://github.com/opensearch-project/.github/blob/main/RESPONSIBILITIES.md#becoming-a-maintainer Signed-off-by: Marc Handalian <marc.handalian@gmail.com>

…ch-project#18254) * Fix matrix_stats aggregation cache conflict by including multiValueMode in equals/hashCode - Added multiValueMode to equals() and hashCode() of MatrixStatsAggregationBuilder - Added serialization/deserialization logic for multiValueMode in writeTo/readFrom - Prevents incorrect aggregator reuse when aggregation mode changes (e.g. AVG ↔ MIN) Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * test: add unit test to verify multiValueMode affects matrix_stats result - Verifies that different multiValueMode settings (e.g., AVG vs MIN) produce different results - Prevents regression of incorrect aggregator reuse due to missing mode in equals/hashCode - Ensures matrix_stats behavior reflects requested aggregation mode Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Make multiValueMode serialization aware of version differences in MatrixStats - Add version check before serializing multiValueMode to maintain backward compatibility - Use AVG as default fallback when reading from pre-3.1.0 versions Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Add roundtrip test to verify serialization and deserialization of MatrixStatsAggregationBuilder - This test ensures that MatrixStatsAggregationBuilder can be correctly - serialized and deserialized when using version >= 3.1.0. - It validates field name and multiValueMode consistency across versions. Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Ensure deserialization fallback to AVG for versions earlier than 2.4.0 - MatrixStatsAggregationBuilder did not serialize multiValueMode before v2.4.0. - This test verifies that deserialization correctly falls back to AVG mode for older versions. Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Verify that equals and hashCode behave correctly for MatrixStatsAggregationBuilder - Adds equality and hashCode consistency checks for different configurations of MatrixStatsAggregationBuilder, including changes in multiValueMode. Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Align deserialization version check for multiValueMode with 3.1.0 Updates the version check in MatrixStatsAggregationBuilder's constructor to use Version.V_3_1_0 when reading multiValueMode from StreamInput. For earlier versions, the fallback defaults to AVG, ensuring compatibility with pre-3.1.0 serialized streams. Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Add matrix stats test for MultiValueMode differences Added SearchIT.testMatrixStatsMultiValueModeEffect to verify that different MultiValueMode settings (AVG vs MIN) produce different matrix stats results for multi-valued fields. Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Add matrix stats multi-value mode test to IndicesRequestCacheIT Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * test: move matrix_stats multiValueMode request cache test to IndicesRequestCacheIT The integration test for verifying request cache behavior of matrix_stats (MultiValueMode.AVG vs MultiValueMode.MIN) was moved from SearchIT to IndicesRequestCacheIT as it directly relates to the caching layer. Also added assertions to verify that: - The first AVG aggregation request triggers a cache miss - The second AVG request hits the cache - The first MIN request is treated as a different query and causes a second miss - The second MIN request results in a hit This aligns with the review suggestion to group request cache-specific tests together and validate distinct cache keys for different MultiValueMode settings. Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Remove matrix-stats module test dependency from server build Removed `testImplementation project(path: ':modules:aggs-matrix-stats')` from `server/build.gradle` to avoid introducing a dependency from core to plugin modules. Tests for matrix-stats should reside within the plugin module itself to maintain proper modular boundaries. Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Add integration test for matrix stats multi-value mode behavior Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Remove matrix stats integration test from core-level test suite Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Restore server/build.gradle to match reactor-netty upgrade commit Restored server/build.gradle to its state at commit c060f92, before test dependencies on aggs-matrix-stats were introduced. This aligns with the modular boundaries that prevent the core server from depending on plugin modules. Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> * Add changelog entry Signed-off-by: Andrew Ross <andrross@amazon.com> --------- Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com> Signed-off-by: Andrew Ross <andrross@amazon.com> Co-authored-by: Andrew Ross <andrross@amazon.com>

…dd support for bucket owner verification (opensearch-project#18312) Signed-off-by: Jay Deng <jayd0104@gmail.com>

As far as I can tell these test cases have never been working as intended. They appear to have been created as a copy-paste error from `testIllegalFsyncInterval`. They passed because the createIndex call was failing due to setting the translog sync interval to an invalid value, which is what the try-catch was expecting. After removing the invalid setting the tests did not work properly for multiple reasons. I believe I have fixed the tests back to the original intent. The reason for digging into this test was to remove the deprecated `createIndex` calls from OpenSearchSingleNodeTestCase which take the now removed type parameter. I will follow up with a separate PR to do that work, as this was a significant enough change to stand on its own. Signed-off-by: Andrew Ross <andrross@amazon.com>

…8331) Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

…EnabledIT (opensearch-project#18236) Signed-off-by: Divyansh Pandey <dpaandey@amazon.com> Co-authored-by: Divyansh Pandey <dpaandey@amazon.com>

…8359) The wrapper-validation-action has been superceded by gradle/actions/wrapper-validation. https://github.com/gradle/wrapper-validation-action?tab=readme-ov-file Signed-off-by: Andrew Ross <andrross@amazon.com>

Signed-off-by: Mayank Sharma <smynk@amazon.com>

…or search traffic (opensearch-project#17791) Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

…project#18327) Signed-off-by: Rakshit Goyal <irakshg@amazon.com>

…ture (opensearch-project#18368) * Bump org.jline:jline in /test/fixtures/hdfs-fixture Bumps [org.jline:jline](https://github.com/jline/jline3) from 3.29.0 to 3.30.3. - [Release notes](https://github.com/jline/jline3/releases) - [Commits](jline/jline3@jline-3.29.0...jline-3.30.3) --- updated-dependencies: - dependency-name: org.jline:jline dependency-version: 3.30.3 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Update changelog Signed-off-by: dependabot[bot] <support@github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…epository-azure (opensearch-project#18369) * Bump com.nimbusds:oauth2-oidc-sdk in /plugins/repository-azure Bumps [com.nimbusds:oauth2-oidc-sdk](https://bitbucket.org/connect2id/oauth-2.0-sdk-with-openid-connect-extensions) from 11.23.1 to 11.25. - [Changelog](https://bitbucket.org/connect2id/oauth-2.0-sdk-with-openid-connect-extensions/src/master/CHANGELOG.txt) - [Commits](https://bitbucket.org/connect2id/oauth-2.0-sdk-with-openid-connect-extensions/branches/compare/11.25..11.23.1) --- updated-dependencies: - dependency-name: com.nimbusds:oauth2-oidc-sdk dependency-version: '11.25' dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> * Updating SHAs Signed-off-by: dependabot[bot] <support@github.com> * Update changelog Signed-off-by: dependabot[bot] <support@github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: Craig Perkins <cwperx@amazon.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>

Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

github-actions · 2025-06-01T18:34:32Z

❌ Gradle check result for 2dcd404: 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?

tandonks · 2025-06-01T18:36:38Z

Refer PR: #18411 The commit history got corrupted in this one

opensearch-trigger-bot · 2025-07-02T15:23:10Z

This PR is stalled because it has been open for 30 days with no activity.

sandeshkr419

Closing in favor of #18411 which is already merged.

tandonks added 2 commits May 14, 2025 08:37

Making changes in InternalValueCount and InternalAvg to support Scrip…

d801f9a

…ted metric in reducing agggregation Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

Adding tests for supporting Scripted Aggregation in reduce for averag…

4c363d5

…e and value count aggregations Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

tandonks requested review from a team, Bukhtawar, CEHENKLE, Rishikesh1159, VachaShah, anasalkouz, andrross, ashking94, cwperks, dbwiddis, gbbafna, jed326, kotwanikunal, mch2, msfroh, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners May 14, 2025 03:20

Fixing build issues

68b6772

Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

tandonks changed the title ~~Making changes in InternalValueCount and InternalAvg to support Scripted metric in reducing agggregation~~ Making changes in InternalValueCount and InternalAvg to support Scripted metric in reducing aggregation May 14, 2025

Adding the change in CHANGELOG.md

91e807a

Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

opensearch-ci-bot mentioned this pull request May 14, 2025

[AUTOCUT] Gradle Check Flaky Test Report for S3BlobStoreRepositoryTests #14299

Open

backslasht suggested changes May 27, 2025

View reviewed changes

Lindsay-00 and others added 19 commits June 1, 2025 20:50

[rule based autotagging] Add Delete Rule API Logic (opensearch-projec…

b5d5992

…t#18184) Signed-off-by: Lingxi Chen <lingxich@amazon.com> Co-authored-by: Lingxi Chen <lingxich@amazon.com>

Remove tests for legacy keystore format (opensearch-project#18154)

16a57b3

These tests have been disabled for over 4 years. Signed-off-by: Andrew Ross <andrross@amazon.com>

Improve failure message in AbstractSimpleTransportTestCase (opensearc…

76f9a06

…h-project#18321) Signed-off-by: Andrew Ross <andrross@amazon.com>

Prepare the codebase for running on JDK-24+ (opensearch-project#18322)

17225a0

Signed-off-by: Andriy Redko <drreta@gmail.com>

Add support for SSE-KMS, remove old server_side_encryption setting, a…

9b836db

…dd support for bucket owner verification (opensearch-project#18312) Signed-off-by: Jay Deng <jayd0104@gmail.com>

Fix MetricAggregatorTests.testStarTreeDocValues (opensearch-project#1…

c7bd4d9

…8331) Signed-off-by: Anthony Leong <aj.leong623@gmail.com>

Fix flaky integration test of ShardsLimitAllocationDeciderRemoteStore…

048d334

…EnabledIT (opensearch-project#18236) Signed-off-by: Divyansh Pandey <dpaandey@amazon.com> Co-authored-by: Divyansh Pandey <dpaandey@amazon.com>

Added changes for full file cache stats. (opensearch-project#17538)

30bf5ed

Signed-off-by: Mayank Sharma <smynk@amazon.com>

Update big5 to use lucene 10.2.1 (opensearch-project#18353)

96c7ded

[Rule based auto tagging] Add WLM action filter to automate tagging f…

05bbe0a

…or search traffic (opensearch-project#17791) Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

Disabling _close API invocation during remote migration. (opensearch-…

1a55bec

…project#18327) Signed-off-by: Rakshit Goyal <irakshg@amazon.com>

Adding the change in CHANGELOG.md

1a10681

Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

Addressed comments around logging and tests

2dcd404

Signed-off-by: Kshitij Tandon <tandonks@amazon.com>

tandonks requested a review from peternied as a code owner June 1, 2025 18:19

tandonks mentioned this pull request Jun 1, 2025

Making changes in InternalValueCount and InternalAvg to support Scripted metric in reducing aggregation #18411

Merged

3 tasks

opensearch-ci-bot mentioned this pull request Jun 12, 2025

[AUTOCUT] Gradle Check Flaky Test Report for InternalEngineTests #15838

Open

opensearch-trigger-bot bot added the stalled Issues that have stalled label Jul 2, 2025

opensearch-ci-bot mentioned this pull request Jul 31, 2025

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreKafkaIT #17693

Open

sandeshkr419 reviewed Aug 14, 2025

View reviewed changes

sandeshkr419 closed this Aug 14, 2025

tkykenmt mentioned this pull request Jan 10, 2026

[feature] Scripted Metric Aggregation tkykenmt/opensearch-feature-explorer#1123

Closed

3 tasks

	- Supporting Scripted Metric Aggregation when reducing aggregations in InternalValueCount and InternalAvg ([18288](https://github.com/opensearch-project/OpenSearch/pull18270)))
	- Supporting Scripted Metric Aggregation when reducing aggregations in InternalValueCount and InternalAvg ([18288](https://github.com/opensearch-project/OpenSearch/pull/18288)))

	count += (long) values[1]; // count is at index 1
	count += Double.valueOf(values[1]).longValue(); // count is at index 1

Conversation

tandonks commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Check List

Uh oh!

github-actions bot commented May 14, 2025

Uh oh!

github-actions bot commented May 14, 2025

Uh oh!

backslasht May 27, 2025

Choose a reason for hiding this comment

Uh oh!

backslasht May 27, 2025

Choose a reason for hiding this comment

Uh oh!

backslasht May 27, 2025

Choose a reason for hiding this comment

Uh oh!

backslasht May 27, 2025

Choose a reason for hiding this comment

Uh oh!

tandonks May 27, 2025

Choose a reason for hiding this comment

Uh oh!

backslasht May 27, 2025

Choose a reason for hiding this comment

Uh oh!

backslasht May 27, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jun 1, 2025

Uh oh!

tandonks commented Jun 1, 2025

Uh oh!

opensearch-trigger-bot bot commented Jul 2, 2025

Uh oh!

sandeshkr419 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

tandonks commented May 14, 2025 •

edited

Loading