Fix MatrixStatsAggregator reuse when mode parameter changes

CHANGELOG.md

@@ -63,6 +63,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Null check field names in QueryStringQueryBuilder ([#18194](https://github.com/opensearch-project/OpenSearch/pull/18194))
 - Avoid NPE if on SnapshotInfo if 'shallow' boolean not present ([#18187](https://github.com/opensearch-project/OpenSearch/issues/18187))
 - Fix 'system call filter not installed' caused when network.host: 0.0.0.0 ([#18309](https://github.com/opensearch-project/OpenSearch/pull/18309))
+- Fix MatrixStatsAggregator reuse when mode parameter changes ([#18242](https://github.com/opensearch-project/OpenSearch/issues/18242))
 
 ### Security
 

modules/aggs-matrix-stats/src/main/java/org/opensearch/search/aggregations/matrix/stats/MatrixStatsAggregationBuilder.java

@@ -31,6 +31,7 @@
 
 package org.opensearch.search.aggregations.matrix.stats;
 
+import org.opensearch.Version;
 import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.core.common.io.stream.StreamOutput;
 import org.opensearch.core.xcontent.ToXContent;
@@ -45,6 +46,7 @@
 
 import java.io.IOException;
 import java.util.Map;
+import java.util.Objects;
 
 public class MatrixStatsAggregationBuilder extends ArrayValuesSourceAggregationBuilder.LeafOnly<MatrixStatsAggregationBuilder> {
     public static final String NAME = "matrix_stats";
@@ -74,11 +76,18 @@ protected AggregationBuilder shallowCopy(AggregatorFactories.Builder factoriesBu
      */
     public MatrixStatsAggregationBuilder(StreamInput in) throws IOException {
         super(in);
+        if (in.getVersion().onOrAfter(Version.V_3_1_0)) {
+            this.multiValueMode = in.readEnum(MultiValueMode.class);
+        } else {
+            this.multiValueMode = MultiValueMode.AVG;
+        }
     }
 
     @Override
-    protected void innerWriteTo(StreamOutput out) {
-        // Do nothing, no extra state to write to stream
+    protected void innerWriteTo(StreamOutput out) throws IOException {
+        if (out.getVersion().onOrAfter(Version.V_3_1_0)) {
+            out.writeEnum(multiValueMode);
+        }
     }
 
     public MatrixStatsAggregationBuilder multiValueMode(MultiValueMode multiValueMode) {
@@ -110,4 +119,18 @@ public XContentBuilder doXContentBody(XContentBuilder builder, ToXContent.Params
     public String getType() {
         return NAME;
     }
+
+    @Override
+    public boolean equals(Object obj) {
+        if (this == obj) return true;
+        if (obj == null || getClass() != obj.getClass()) return false;
+        if (super.equals(obj) == false) return false;
+        MatrixStatsAggregationBuilder other = (MatrixStatsAggregationBuilder) obj;
+        return multiValueMode == other.multiValueMode;
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hash(super.hashCode(), multiValueMode);
+    }
 }

modules/aggs-matrix-stats/src/test/java/org/opensearch/search/aggregations/matrix/MatrixStatsIT.java

@@ -0,0 +1,93 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.search.aggregations.matrix;
+
+import org.opensearch.cluster.metadata.IndexMetadata;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.index.IndexSettings;
+import org.opensearch.index.cache.request.RequestCacheStats;
+import org.opensearch.indices.IndicesRequestCache;
+import org.opensearch.search.MultiValueMode;
+import org.opensearch.search.aggregations.matrix.stats.MatrixStatsAggregationBuilder;
+import org.opensearch.test.OpenSearchIntegTestCase;
+import org.opensearch.test.ParameterizedStaticSettingsOpenSearchIntegTestCase;
+import org.opensearch.transport.client.Client;
+
+import java.util.List;
+
+import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
+
+@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, supportsDedicatedMasters = false)
+public class MatrixStatsIT extends ParameterizedStaticSettingsOpenSearchIntegTestCase {
+    public MatrixStatsIT(Settings nodeSettings) {
+        super(nodeSettings);
+    }
+
+    public void testMatrixStatsMultiValueModeEffect() throws Exception {
+        String index = "test_matrix_stats_multimode";
+        Client client = client();
+
+        assertAcked(
+            client.admin()
+                .indices()
+                .prepareCreate(index)
+                .setSettings(
+                    Settings.builder()
+                        .put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), -1)
+                        .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                        .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
+                        .put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true)
+                )
+                .get()
+        );
+
+        client.prepareIndex(index).setId("1").setSource("num", List.of(10, 30), "num2", List.of(40, 60)).setWaitForActiveShards(1).get();
+        client.admin().indices().prepareRefresh(index).get();
+
+        MatrixStatsAggregationBuilder avgAgg = new MatrixStatsAggregationBuilder("agg_avg").fields(List.of("num", "num2"))
+            .multiValueMode(MultiValueMode.AVG);
+
+        client.prepareSearch(index).setSize(0).setRequestCache(true).addAggregation(avgAgg).get();
+
+        RequestCacheStats stats1 = getRequestCacheStats(client, index);
+        long hit1 = stats1.getHitCount();
+        long miss1 = stats1.getMissCount();
+
+        client.prepareSearch(index).setSize(0).setRequestCache(true).addAggregation(avgAgg).get();
+
+        RequestCacheStats stats2 = getRequestCacheStats(client, index);
+        long hit2 = stats2.getHitCount();
+        long miss2 = stats2.getMissCount();
+
+        MatrixStatsAggregationBuilder minAgg = new MatrixStatsAggregationBuilder("agg_min").fields(List.of("num", "num2"))
+            .multiValueMode(MultiValueMode.MIN);
+
+        client.prepareSearch(index).setSize(0).setRequestCache(true).addAggregation(minAgg).get();
+
+        RequestCacheStats stats3 = getRequestCacheStats(client, index);
+        long hit3 = stats3.getHitCount();
+        long miss3 = stats3.getMissCount();
+
+        client.prepareSearch(index).setSize(0).setRequestCache(true).addAggregation(minAgg).get();
+
+        RequestCacheStats stats4 = getRequestCacheStats(client, index);
+        long hit4 = stats4.getHitCount();
+        long miss4 = stats4.getMissCount();
+
+        assertEquals("Expected 1 cache miss for first AVG request", 1, miss1);
+        assertEquals("Expected 1 cache hit for second AVG request", hit1 + 1, hit2);
+        assertEquals("Expected 1 cache miss for first MIN request", miss1 + 1, miss3);
+        assertEquals("Expected 1 cache hit for second MIN request", hit2 + 1, hit4);
+        assertEquals("Expected no additional cache misses for second MIN request", miss3, miss4);
+    }
+
+    private static RequestCacheStats getRequestCacheStats(Client client, String index) {
+        return client.admin().indices().prepareStats(index).setRequestCache(true).get().getTotal().getRequestCache();
+    }
+}

modules/aggs-matrix-stats/src/test/java/org/opensearch/search/aggregations/matrix/stats/MatrixStatsAggregatorTests.java

@@ -41,12 +41,17 @@
 import org.apache.lucene.store.Directory;
 import org.apache.lucene.tests.index.RandomIndexWriter;
 import org.apache.lucene.util.NumericUtils;
+import org.opensearch.Version;
+import org.opensearch.common.io.stream.BytesStreamOutput;
+import org.opensearch.core.common.io.stream.StreamInput;
 import org.opensearch.index.mapper.MappedFieldType;
 import org.opensearch.index.mapper.NumberFieldMapper;
 import org.opensearch.plugins.SearchPlugin;
+import org.opensearch.search.MultiValueMode;
 import org.opensearch.search.aggregations.AggregatorTestCase;
 import org.opensearch.search.aggregations.matrix.MatrixAggregationModulePlugin;
 
+import java.io.IOException;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.List;
@@ -126,6 +131,90 @@ public void testTwoFields() throws Exception {
         }
     }
 
+    public void testMultiValueModeAffectsResult() throws Exception {
+        String field = "grades";
+        MappedFieldType ft = new NumberFieldMapper.NumberFieldType(field, NumberFieldMapper.NumberType.DOUBLE);
+
+        try (Directory directory = newDirectory(); RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
+            Document doc = new Document();
+            doc.add(new SortedNumericDocValuesField(field, NumericUtils.doubleToSortableLong(1.0)));
+            doc.add(new SortedNumericDocValuesField(field, NumericUtils.doubleToSortableLong(3.0)));
+            doc.add(new SortedNumericDocValuesField(field, NumericUtils.doubleToSortableLong(5.0)));
+            indexWriter.addDocument(doc);
+
+            try (IndexReader reader = indexWriter.getReader()) {
+                IndexSearcher searcher = new IndexSearcher(reader);
+
+                MatrixStatsAggregationBuilder avgAgg = new MatrixStatsAggregationBuilder("avg_agg").fields(Collections.singletonList(field))
+                    .multiValueMode(MultiValueMode.AVG);
+
+                MatrixStatsAggregationBuilder minAgg = new MatrixStatsAggregationBuilder("min_agg").fields(Collections.singletonList(field))
+                    .multiValueMode(MultiValueMode.MIN);
+
+                InternalMatrixStats avgStats = searchAndReduce(searcher, new MatchAllDocsQuery(), avgAgg, ft);
+                InternalMatrixStats minStats = searchAndReduce(searcher, new MatchAllDocsQuery(), minAgg, ft);
+
+                double avg = avgStats.getMean(field);
+                double min = minStats.getMean(field);
+
+                assertNotEquals("AVG and MIN mode should yield different means", avg, min, 0.0001);
+            }
+        }
+    }
+
+    public void testSerializationDeserialization() throws IOException {
+        MatrixStatsAggregationBuilder original = new MatrixStatsAggregationBuilder("test").fields(Collections.singletonList("field"))
+            .multiValueMode(MultiValueMode.MIN);
+
+        // Serialize
+        BytesStreamOutput out = new BytesStreamOutput();
+        out.setVersion(Version.V_3_1_0);
+        original.writeTo(out);
+
+        // Deserialize
+        StreamInput in = out.bytes().streamInput();
+        in.setVersion(Version.V_3_1_0);
+        MatrixStatsAggregationBuilder deserialized = new MatrixStatsAggregationBuilder(in);
+
+        assertEquals(original.getName(), deserialized.getName());
+        assertEquals(original.fields(), deserialized.fields());
+        assertEquals(original.multiValueMode(), deserialized.multiValueMode());
+    }
+
+    public void testDeserializationFallbackToAvg() throws IOException {
+        MatrixStatsAggregationBuilder original = new MatrixStatsAggregationBuilder("test").fields(Collections.singletonList("field"));
+
+        // Serialize with V_2_3_0 (fallback required)
+        BytesStreamOutput out = new BytesStreamOutput();
+        out.setVersion(Version.V_2_3_0);
+        original.writeTo(out);
+
+        StreamInput in = out.bytes().streamInput();
+        in.setVersion(Version.V_2_3_0);
+        MatrixStatsAggregationBuilder deserialized = new MatrixStatsAggregationBuilder(in);
+
+        assertEquals(MultiValueMode.AVG, deserialized.multiValueMode());
+    }
+
+    public void testEqualsAndHashCode() {
+        MatrixStatsAggregationBuilder agg1 = new MatrixStatsAggregationBuilder("agg").fields(Collections.singletonList("field"))
+            .multiValueMode(MultiValueMode.AVG);
+
+        MatrixStatsAggregationBuilder agg2 = new MatrixStatsAggregationBuilder("agg").fields(Collections.singletonList("field"))
+            .multiValueMode(MultiValueMode.AVG);
+
+        MatrixStatsAggregationBuilder agg3 = new MatrixStatsAggregationBuilder("agg").fields(Collections.singletonList("field"))
+            .multiValueMode(MultiValueMode.MIN);
+
+        // equals
+        assertEquals(agg1, agg2);
+        assertNotEquals(agg1, agg3);
+
+        // hashCode
+        assertEquals(agg1.hashCode(), agg2.hashCode());
+        assertNotEquals(agg1.hashCode(), agg3.hashCode());
+    }
+
     @Override
     protected List<SearchPlugin> getSearchPlugins() {
         return Collections.singletonList(new MatrixAggregationModulePlugin());