Skip to content

Conversation

@jinlee1703
Copy link
Contributor

@jinlee1703 jinlee1703 commented May 11, 2025

Description

This PR fixes an issue where MatrixStatsAggregator was incorrectly reused across different aggregation mode values (e.g., avg vs min), as reported in #18242.

Root Cause

The root cause was that the mode parameter (MultiValueMode) was not included in MatrixStatsAggregationBuilder's equals() and hashCode() methods. This led to an aggregator cache key collision, and incorrect reuse of the previous aggregator even when the mode changed.

Fix Summary

  • Added multiValueMode to equals() and hashCode() in MatrixStatsAggregationBuilder
  • Added serialization and deserialization logic for multiValueMode in writeTo() and constructor
  • This ensures that aggregators are re-initialized when the mode changes

This fix ensures the correct behavior of matrix_stats aggregation across different mode values, especially for multi-valued fields.


Related Issue

Resolves #18242


Check List

  • Fix verified with multiple mode-switching queries (avgmin)
  • Tests added or updated
  • API changes require documentation updates (N/A)
  • All code is compliant with Apache 2.0 licensing terms

@github-actions
Copy link
Contributor

❌ Gradle check result for 386ab58: 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
Copy link
Contributor

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

@jinlee1703 jinlee1703 force-pushed the fix/matrix-stats-mode-cache branch from 391c80c to d1a6bfc Compare May 12, 2025 15:33
@github-actions
Copy link
Contributor

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

@jinlee1703 jinlee1703 force-pushed the fix/matrix-stats-mode-cache branch from d1a6bfc to bfe7206 Compare May 12, 2025 15:55
@github-actions
Copy link
Contributor

❕ Gradle check result for bfe7206: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@codecov
Copy link

codecov bot commented May 12, 2025

Codecov Report

Attention: Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 72.58%. Comparing base (631bed4) to head (a55cd83).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ns/matrix/stats/MatrixStatsAggregationBuilder.java 72.72% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #18254      +/-   ##
============================================
+ Coverage     72.53%   72.58%   +0.04%     
- Complexity    67396    67465      +69     
============================================
  Files          5492     5492              
  Lines        311127   311138      +11     
  Branches      45224    45228       +4     
============================================
+ Hits         225680   225831     +151     
+ Misses        67063    66900     -163     
- Partials      18384    18407      +23     

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

@github-actions
Copy link
Contributor

❌ Gradle check result for 827a51f: 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
Copy link
Contributor

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

Copy link
Contributor

@sgup432 sgup432 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. Lets add an integration test for this as well.
Where we verify that sending query with different modes doesn't return the same response.

Request Cache IT test would an appropriate place to add this test case

@github-actions
Copy link
Contributor

✅ Gradle check result for f671a50: SUCCESS

@jinlee1703 jinlee1703 requested review from andrross and sgup432 May 13, 2025 05:35
@jinlee1703 jinlee1703 force-pushed the fix/matrix-stats-mode-cache branch from 870b41f to de60c08 Compare May 14, 2025 04:44
@github-actions
Copy link
Contributor

❌ Gradle check result for de60c08: 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
Copy link
Contributor

✅ Gradle check result for ba8fe91: SUCCESS

@jainankitk
Copy link
Contributor

Will be able to merge this PR, once DCO check concern is addressed.

jinlee1703 and others added 15 commits May 22, 2025 11:05
…de 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>
- 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>
…rixStats

- 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>
…rixStatsAggregationBuilder

- 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>
- 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>
…gationBuilder

- Adds equality and hashCode consistency checks for different configurations of MatrixStatsAggregationBuilder, including changes in multiValueMode.

Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com>
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>
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>
Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com>
…equestCacheIT

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>
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>
Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com>
Signed-off-by: Jinwoo Lee <jinlee1703@gmail.com>
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>
Signed-off-by: Andrew Ross <andrross@amazon.com>
@jinlee1703 jinlee1703 force-pushed the fix/matrix-stats-mode-cache branch from ba8fe91 to a55cd83 Compare May 22, 2025 02:07
@jinlee1703
Copy link
Contributor Author

jinlee1703 commented May 22, 2025

@andrross Thanks for pointing that out!
I fixed the DCO issue by rewriting the sign-off via an interactive rebase (including the first commit manually).

@github-actions
Copy link
Contributor

✅ Gradle check result for a55cd83: SUCCESS

@jainankitk jainankitk merged commit 6dd6f03 into opensearch-project:main May 22, 2025
30 checks passed
@andrross
Copy link
Member

andrross commented May 22, 2025

@jainankitk Is this bug present in 2.19? If so, should the fix be backported for the next maintenance release?

Edit: I believe the answer is no. This is a very long-standing bug (probably existed pre-fork), so not worth the risk of regression by patching back into 2.19 in my opinion.

tandonks pushed a commit to tandonks/OpenSearch that referenced this pull request Jun 1, 2025
…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>
Gagan6164 pushed a commit to Gagan6164/OpenSearch that referenced this pull request Jun 8, 2025
…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>
Gagan6164 pushed a commit to Gagan6164/OpenSearch that referenced this pull request Jun 8, 2025
…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>
neuenfeldttj added a commit to neuenfeldttj/OpenSearch that referenced this pull request Jun 26, 2025
…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>Signed-off-by: TJ Neuenfeldt <tjneu@amazon.com>
neuenfeldttj pushed a commit to neuenfeldttj/OpenSearch that referenced this pull request Jun 26, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Search:Aggregations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Caching does not consider mode parameter for matrix-stats aggregation

4 participants