Skip to content

Commit 6dd6f03

Browse files
jinlee1703andrross
jinlee1703
and
andrross
authored
Fix MatrixStatsAggregator reuse when mode parameter changes (#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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Add matrix stats multi-value mode test to IndicesRequestCacheIT Signed-off-by: Jinwoo Lee <[email protected]> * 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 <[email protected]> * 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 <[email protected]> * Add integration test for matrix stats multi-value mode behavior Signed-off-by: Jinwoo Lee <[email protected]> * Remove matrix stats integration test from core-level test suite Signed-off-by: Jinwoo Lee <[email protected]> * 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 <[email protected]> * Add changelog entry Signed-off-by: Andrew Ross <[email protected]> --------- Signed-off-by: Jinwoo Lee <[email protected]> Signed-off-by: Andrew Ross <[email protected]> Co-authored-by: Andrew Ross <[email protected]>
1 parent d723db8 commit 6dd6f03

File tree

4 files changed

+208
-2
lines changed

4 files changed

+208
-2
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
6363
- Null check field names in QueryStringQueryBuilder ([#18194](https://github.com/opensearch-project/OpenSearch/pull/18194))
6464
- Avoid NPE if on SnapshotInfo if 'shallow' boolean not present ([#18187](https://github.com/opensearch-project/OpenSearch/issues/18187))
6565
- Fix 'system call filter not installed' caused when network.host: 0.0.0.0 ([#18309](https://github.com/opensearch-project/OpenSearch/pull/18309))
66+
- Fix MatrixStatsAggregator reuse when mode parameter changes ([#18242](https://github.com/opensearch-project/OpenSearch/issues/18242))
6667

6768
### Security
6869

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

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131

3232
package org.opensearch.search.aggregations.matrix.stats;
3333

34+
import org.opensearch.Version;
3435
import org.opensearch.core.common.io.stream.StreamInput;
3536
import org.opensearch.core.common.io.stream.StreamOutput;
3637
import org.opensearch.core.xcontent.ToXContent;
@@ -45,6 +46,7 @@
4546

4647
import java.io.IOException;
4748
import java.util.Map;
49+
import java.util.Objects;
4850

4951
public class MatrixStatsAggregationBuilder extends ArrayValuesSourceAggregationBuilder.LeafOnly<MatrixStatsAggregationBuilder> {
5052
public static final String NAME = "matrix_stats";
@@ -74,11 +76,18 @@ protected AggregationBuilder shallowCopy(AggregatorFactories.Builder factoriesBu
7476
*/
7577
public MatrixStatsAggregationBuilder(StreamInput in) throws IOException {
7678
super(in);
79+
if (in.getVersion().onOrAfter(Version.V_3_1_0)) {
80+
this.multiValueMode = in.readEnum(MultiValueMode.class);
81+
} else {
82+
this.multiValueMode = MultiValueMode.AVG;
83+
}
7784
}
7885

7986
@Override
80-
protected void innerWriteTo(StreamOutput out) {
81-
// Do nothing, no extra state to write to stream
87+
protected void innerWriteTo(StreamOutput out) throws IOException {
88+
if (out.getVersion().onOrAfter(Version.V_3_1_0)) {
89+
out.writeEnum(multiValueMode);
90+
}
8291
}
8392

8493
public MatrixStatsAggregationBuilder multiValueMode(MultiValueMode multiValueMode) {
@@ -110,4 +119,18 @@ public XContentBuilder doXContentBody(XContentBuilder builder, ToXContent.Params
110119
public String getType() {
111120
return NAME;
112121
}
122+
123+
@Override
124+
public boolean equals(Object obj) {
125+
if (this == obj) return true;
126+
if (obj == null || getClass() != obj.getClass()) return false;
127+
if (super.equals(obj) == false) return false;
128+
MatrixStatsAggregationBuilder other = (MatrixStatsAggregationBuilder) obj;
129+
return multiValueMode == other.multiValueMode;
130+
}
131+
132+
@Override
133+
public int hashCode() {
134+
return Objects.hash(super.hashCode(), multiValueMode);
135+
}
113136
}

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

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
/*
2+
* SPDX-License-Identifier: Apache-2.0
3+
*
4+
* The OpenSearch Contributors require contributions made to
5+
* this file be licensed under the Apache-2.0 license or a
6+
* compatible open source license.
7+
*/
8+
9+
package org.opensearch.search.aggregations.matrix;
10+
11+
import org.opensearch.cluster.metadata.IndexMetadata;
12+
import org.opensearch.common.settings.Settings;
13+
import org.opensearch.index.IndexSettings;
14+
import org.opensearch.index.cache.request.RequestCacheStats;
15+
import org.opensearch.indices.IndicesRequestCache;
16+
import org.opensearch.search.MultiValueMode;
17+
import org.opensearch.search.aggregations.matrix.stats.MatrixStatsAggregationBuilder;
18+
import org.opensearch.test.OpenSearchIntegTestCase;
19+
import org.opensearch.test.ParameterizedStaticSettingsOpenSearchIntegTestCase;
20+
import org.opensearch.transport.client.Client;
21+
22+
import java.util.List;
23+
24+
import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked;
25+
26+
@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST, numDataNodes = 0, supportsDedicatedMasters = false)
27+
public class MatrixStatsIT extends ParameterizedStaticSettingsOpenSearchIntegTestCase {
28+
public MatrixStatsIT(Settings nodeSettings) {
29+
super(nodeSettings);
30+
}
31+
32+
public void testMatrixStatsMultiValueModeEffect() throws Exception {
33+
String index = "test_matrix_stats_multimode";
34+
Client client = client();
35+
36+
assertAcked(
37+
client.admin()
38+
.indices()
39+
.prepareCreate(index)
40+
.setSettings(
41+
Settings.builder()
42+
.put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), -1)
43+
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
44+
.put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)
45+
.put(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING.getKey(), true)
46+
)
47+
.get()
48+
);
49+
50+
client.prepareIndex(index).setId("1").setSource("num", List.of(10, 30), "num2", List.of(40, 60)).setWaitForActiveShards(1).get();
51+
client.admin().indices().prepareRefresh(index).get();
52+
53+
MatrixStatsAggregationBuilder avgAgg = new MatrixStatsAggregationBuilder("agg_avg").fields(List.of("num", "num2"))
54+
.multiValueMode(MultiValueMode.AVG);
55+
56+
client.prepareSearch(index).setSize(0).setRequestCache(true).addAggregation(avgAgg).get();
57+
58+
RequestCacheStats stats1 = getRequestCacheStats(client, index);
59+
long hit1 = stats1.getHitCount();
60+
long miss1 = stats1.getMissCount();
61+
62+
client.prepareSearch(index).setSize(0).setRequestCache(true).addAggregation(avgAgg).get();
63+
64+
RequestCacheStats stats2 = getRequestCacheStats(client, index);
65+
long hit2 = stats2.getHitCount();
66+
long miss2 = stats2.getMissCount();
67+
68+
MatrixStatsAggregationBuilder minAgg = new MatrixStatsAggregationBuilder("agg_min").fields(List.of("num", "num2"))
69+
.multiValueMode(MultiValueMode.MIN);
70+
71+
client.prepareSearch(index).setSize(0).setRequestCache(true).addAggregation(minAgg).get();
72+
73+
RequestCacheStats stats3 = getRequestCacheStats(client, index);
74+
long hit3 = stats3.getHitCount();
75+
long miss3 = stats3.getMissCount();
76+
77+
client.prepareSearch(index).setSize(0).setRequestCache(true).addAggregation(minAgg).get();
78+
79+
RequestCacheStats stats4 = getRequestCacheStats(client, index);
80+
long hit4 = stats4.getHitCount();
81+
long miss4 = stats4.getMissCount();
82+
83+
assertEquals("Expected 1 cache miss for first AVG request", 1, miss1);
84+
assertEquals("Expected 1 cache hit for second AVG request", hit1 + 1, hit2);
85+
assertEquals("Expected 1 cache miss for first MIN request", miss1 + 1, miss3);
86+
assertEquals("Expected 1 cache hit for second MIN request", hit2 + 1, hit4);
87+
assertEquals("Expected no additional cache misses for second MIN request", miss3, miss4);
88+
}
89+
90+
private static RequestCacheStats getRequestCacheStats(Client client, String index) {
91+
return client.admin().indices().prepareStats(index).setRequestCache(true).get().getTotal().getRequestCache();
92+
}
93+
}

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

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,17 @@
4141
import org.apache.lucene.store.Directory;
4242
import org.apache.lucene.tests.index.RandomIndexWriter;
4343
import org.apache.lucene.util.NumericUtils;
44+
import org.opensearch.Version;
45+
import org.opensearch.common.io.stream.BytesStreamOutput;
46+
import org.opensearch.core.common.io.stream.StreamInput;
4447
import org.opensearch.index.mapper.MappedFieldType;
4548
import org.opensearch.index.mapper.NumberFieldMapper;
4649
import org.opensearch.plugins.SearchPlugin;
50+
import org.opensearch.search.MultiValueMode;
4751
import org.opensearch.search.aggregations.AggregatorTestCase;
4852
import org.opensearch.search.aggregations.matrix.MatrixAggregationModulePlugin;
4953

54+
import java.io.IOException;
5055
import java.util.Arrays;
5156
import java.util.Collections;
5257
import java.util.List;
@@ -126,6 +131,90 @@ public void testTwoFields() throws Exception {
126131
}
127132
}
128133

134+
public void testMultiValueModeAffectsResult() throws Exception {
135+
String field = "grades";
136+
MappedFieldType ft = new NumberFieldMapper.NumberFieldType(field, NumberFieldMapper.NumberType.DOUBLE);
137+
138+
try (Directory directory = newDirectory(); RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory)) {
139+
Document doc = new Document();
140+
doc.add(new SortedNumericDocValuesField(field, NumericUtils.doubleToSortableLong(1.0)));
141+
doc.add(new SortedNumericDocValuesField(field, NumericUtils.doubleToSortableLong(3.0)));
142+
doc.add(new SortedNumericDocValuesField(field, NumericUtils.doubleToSortableLong(5.0)));
143+
indexWriter.addDocument(doc);
144+
145+
try (IndexReader reader = indexWriter.getReader()) {
146+
IndexSearcher searcher = new IndexSearcher(reader);
147+
148+
MatrixStatsAggregationBuilder avgAgg = new MatrixStatsAggregationBuilder("avg_agg").fields(Collections.singletonList(field))
149+
.multiValueMode(MultiValueMode.AVG);
150+
151+
MatrixStatsAggregationBuilder minAgg = new MatrixStatsAggregationBuilder("min_agg").fields(Collections.singletonList(field))
152+
.multiValueMode(MultiValueMode.MIN);
153+
154+
InternalMatrixStats avgStats = searchAndReduce(searcher, new MatchAllDocsQuery(), avgAgg, ft);
155+
InternalMatrixStats minStats = searchAndReduce(searcher, new MatchAllDocsQuery(), minAgg, ft);
156+
157+
double avg = avgStats.getMean(field);
158+
double min = minStats.getMean(field);
159+
160+
assertNotEquals("AVG and MIN mode should yield different means", avg, min, 0.0001);
161+
}
162+
}
163+
}
164+
165+
public void testSerializationDeserialization() throws IOException {
166+
MatrixStatsAggregationBuilder original = new MatrixStatsAggregationBuilder("test").fields(Collections.singletonList("field"))
167+
.multiValueMode(MultiValueMode.MIN);
168+
169+
// Serialize
170+
BytesStreamOutput out = new BytesStreamOutput();
171+
out.setVersion(Version.V_3_1_0);
172+
original.writeTo(out);
173+
174+
// Deserialize
175+
StreamInput in = out.bytes().streamInput();
176+
in.setVersion(Version.V_3_1_0);
177+
MatrixStatsAggregationBuilder deserialized = new MatrixStatsAggregationBuilder(in);
178+
179+
assertEquals(original.getName(), deserialized.getName());
180+
assertEquals(original.fields(), deserialized.fields());
181+
assertEquals(original.multiValueMode(), deserialized.multiValueMode());
182+
}
183+
184+
public void testDeserializationFallbackToAvg() throws IOException {
185+
MatrixStatsAggregationBuilder original = new MatrixStatsAggregationBuilder("test").fields(Collections.singletonList("field"));
186+
187+
// Serialize with V_2_3_0 (fallback required)
188+
BytesStreamOutput out = new BytesStreamOutput();
189+
out.setVersion(Version.V_2_3_0);
190+
original.writeTo(out);
191+
192+
StreamInput in = out.bytes().streamInput();
193+
in.setVersion(Version.V_2_3_0);
194+
MatrixStatsAggregationBuilder deserialized = new MatrixStatsAggregationBuilder(in);
195+
196+
assertEquals(MultiValueMode.AVG, deserialized.multiValueMode());
197+
}
198+
199+
public void testEqualsAndHashCode() {
200+
MatrixStatsAggregationBuilder agg1 = new MatrixStatsAggregationBuilder("agg").fields(Collections.singletonList("field"))
201+
.multiValueMode(MultiValueMode.AVG);
202+
203+
MatrixStatsAggregationBuilder agg2 = new MatrixStatsAggregationBuilder("agg").fields(Collections.singletonList("field"))
204+
.multiValueMode(MultiValueMode.AVG);
205+
206+
MatrixStatsAggregationBuilder agg3 = new MatrixStatsAggregationBuilder("agg").fields(Collections.singletonList("field"))
207+
.multiValueMode(MultiValueMode.MIN);
208+
209+
// equals
210+
assertEquals(agg1, agg2);
211+
assertNotEquals(agg1, agg3);
212+
213+
// hashCode
214+
assertEquals(agg1.hashCode(), agg2.hashCode());
215+
assertNotEquals(agg1.hashCode(), agg3.hashCode());
216+
}
217+
129218
@Override
130219
protected List<SearchPlugin> getSearchPlugins() {
131220
return Collections.singletonList(new MatrixAggregationModulePlugin());

0 commit comments

Comments
 (0)