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

tandonks · 2025-06-01T18:32:11Z

Description

We had introduced the support for searching rollup and raw indices together in this opensearch-project/index-management#1268 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 PR: #18288

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.

github-actions · 2025-06-01T18:53:29Z

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

sandeshkr419

I saw the comment on https://github.com/opensearch-project/index-management/blame/main/src/main/kotlin/org/opensearch/indexmanagement/rollup/util/RollupUtils.kt#L263

How early was it tested that the value count aggregation does not fulfil what you need?
I see some comments on type conversions, which I guess should be doable for value counts given that value counts will be a whole number for sure (I guess).

Now I don't fully understand your use-case in IM, but is it feasible to sum & value-count from core as it is and then calculate sum/value-count in IM plugin? Also, please check my comment in InternalAvg.java as well.

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

…ted metric in reducing agggregation Signed-off-by: Kshitij Tandon <[email protected]>

…e and value count aggregations Signed-off-by: Kshitij Tandon <[email protected]>

Signed-off-by: Kshitij Tandon <[email protected]>

github-actions · 2025-08-05T07:49:21Z

❌ Gradle check result for 41bd4e5: 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 <[email protected]>

github-actions · 2025-08-05T08:04:47Z

❌ Gradle check result for 9eaeccb: 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 <[email protected]>

github-actions · 2025-08-05T10:35:52Z

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

github-actions · 2025-08-05T12:01:00Z

✅ Gradle check result for 594c2c3: SUCCESS

codecov · 2025-08-05T12:01:44Z

Codecov Report

❌ Patch coverage is 73.52941% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.92%. Comparing base (cc20ac7) to head (221f56b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...earch/search/aggregations/metrics/ScriptedAvg.java	46.15%	7 Missing ⚠️
...earch/search/aggregations/metrics/InternalAvg.java	91.66%	0 Missing and 1 partial ⚠️
...earch/aggregations/metrics/InternalValueCount.java	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18411      +/-   ##
============================================
+ Coverage     72.87%   72.92%   +0.05%     
- Complexity    69326    69362      +36     
============================================
  Files          5641     5642       +1     
  Lines        318474   318504      +30     
  Branches      46062    46068       +6     
============================================
+ Hits         232076   232276     +200     
+ Misses        67581    67438     -143     
+ Partials      18817    18790      -27

☔ 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.

Signed-off-by: Kshitij Tandon <[email protected]>

github-actions · 2025-08-06T10:29:15Z

✅ Gradle check result for 221f56b: SUCCESS

sandeshkr419

@tandonks Since the changes have already been merged, can you please fix the license header in a new PR?

Thanks!

sandeshkr419 · 2025-08-06T18:57:30Z

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

+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/*
+ * Modifications Copyright OpenSearch Contributors. See
+ * GitHub history for details.
+ */
+


Since this is a new file, can you please fix the license header - you only need lines 1-7 as license header and not the remaining Elasticsearch license lines 9-31.

…ted metric in reducing aggregation (opensearch-project#18411) * Making changes in InternalValueCount and InternalAvg to support Scripted metric in reducing agggregation Signed-off-by: Kshitij Tandon <[email protected]> * Adding tests for supporting Scripted Aggregation in reduce for average and value count aggregations Signed-off-by: Kshitij Tandon <[email protected]> * Fixing build issues Signed-off-by: Kshitij Tandon <[email protected]> * Addressed comments around logging and tests Signed-off-by: Kshitij Tandon <[email protected]> * Making changes to CHANGELOG.md Signed-off-by: Kshitij Tandon <[email protected]> * Adding a new class ScriptedAvg for reduce Signed-off-by: Kshitij Tandon <[email protected]> * Adding changes to CHANGELOG Signed-off-by: Kshitij Tandon <[email protected]> * Adding javadocs Signed-off-by: Kshitij Tandon <[email protected]> * Fixing violations Signed-off-by: Kshitij Tandon <[email protected]> --------- Signed-off-by: Kshitij Tandon <[email protected]>

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 June 1, 2025 18:32

tandonks changed the title ~~Tandonks test 2~~ Making changes in InternalValueCount and InternalAvg to support Scripted metric in reducing aggregation Jun 1, 2025

tandonks mentioned this pull request Jun 1, 2025

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

Closed

3 tasks

sandeshkr419 reviewed Jul 1, 2025

View reviewed changes

server/src/main/java/org/opensearch/search/aggregations/metrics/InternalAvg.java Outdated Show resolved Hide resolved

tandonks requested a review from peternied as a code owner August 5, 2025 07:12

tandonks force-pushed the tandonks-test-2 branch 2 times, most recently from 001f7e8 to e3811a7 Compare August 5, 2025 07:32

tandonks added 2 commits August 5, 2025 13:03

Making changes in InternalValueCount and InternalAvg to support Scrip…

44d09ce

…ted metric in reducing agggregation Signed-off-by: Kshitij Tandon <[email protected]>

Adding tests for supporting Scripted Aggregation in reduce for averag…

dc69326

…e and value count aggregations Signed-off-by: Kshitij Tandon <[email protected]>

tandonks added 3 commits August 5, 2025 13:03

Fixing build issues

16d2714

Signed-off-by: Kshitij Tandon <[email protected]>

Addressed comments around logging and tests

338818e

Signed-off-by: Kshitij Tandon <[email protected]>

Making changes to CHANGELOG.md

901fefd

Signed-off-by: Kshitij Tandon <[email protected]>

tandonks force-pushed the tandonks-test-2 branch from e3811a7 to d748ab9 Compare August 5, 2025 07:34

Adding a new class ScriptedAvg for reduce

41bd4e5

Signed-off-by: Kshitij Tandon <[email protected]>

tandonks force-pushed the tandonks-test-2 branch from d748ab9 to 41bd4e5 Compare August 5, 2025 07:35

Adding changes to CHANGELOG

9eaeccb

Signed-off-by: Kshitij Tandon <[email protected]>

tandonks added 2 commits August 5, 2025 16:04

Adding javadocs

51b9ee6

Signed-off-by: Kshitij Tandon <[email protected]>

Fixing violations

594c2c3

Signed-off-by: Kshitij Tandon <[email protected]>

tandonks force-pushed the tandonks-test-2 branch from c1b4da4 to 594c2c3 Compare August 5, 2025 10:36

bharath-techie approved these changes Aug 6, 2025

View reviewed changes

Merge branch 'main' into tandonks-test-2

221f56b

Signed-off-by: Kshitij Tandon <[email protected]>

bharath-techie merged commit 2fedeb5 into opensearch-project:main Aug 6, 2025
31 checks passed

sandeshkr419 reviewed Aug 6, 2025

View reviewed changes

This was referenced Aug 9, 2025

Using Scripted Avg Class in AvgAggregationBuilder in place of double opensearch-project/index-management#1460

Merged

Adding ScriptedAvg class to painless spi to allowlist usage from plugins #19006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

tandonks commented Jun 1, 2025

Uh oh!

github-actions bot commented Jun 1, 2025

Uh oh!

sandeshkr419 left a comment

Uh oh!

Uh oh!

github-actions bot commented Aug 5, 2025

Uh oh!

github-actions bot commented Aug 5, 2025

Uh oh!

github-actions bot commented Aug 5, 2025

Uh oh!

github-actions bot commented Aug 5, 2025

Uh oh!

codecov bot commented Aug 5, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Aug 6, 2025

Uh oh!

Uh oh!

sandeshkr419 left a comment •

edited

Loading

Uh oh!

sandeshkr419 Aug 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

Uh oh!

Conversation

tandonks commented Jun 1, 2025

Description

Related Issues

Check List

Uh oh!

github-actions bot commented Jun 1, 2025

Uh oh!

sandeshkr419 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Aug 5, 2025

Uh oh!

github-actions bot commented Aug 5, 2025

Uh oh!

github-actions bot commented Aug 5, 2025

Uh oh!

github-actions bot commented Aug 5, 2025

Uh oh!

codecov bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Aug 6, 2025

Uh oh!

Uh oh!

sandeshkr419 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandeshkr419 Aug 6, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Aug 5, 2025 •

edited

Loading

sandeshkr419 left a comment •

edited

Loading