From 958cf7cf0c84abfc8ae740cef1f033ecc31ad348 Mon Sep 17 00:00:00 2001 From: Prudhvi Godithi Date: Tue, 15 Jul 2025 13:52:06 -0700 Subject: [PATCH 1/3] Approximation Framework bug fix Signed-off-by: Prudhvi Godithi --- CHANGELOG.md | 1 + .../search/approximate/ApproximateMatchAllQuery.java | 3 +++ .../search/approximate/ApproximatePointRangeQuery.java | 3 +++ 3 files changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d04c01edc47d9..3c0ed80bd1743 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Pass index settings to system ingest processor factories. ([#18708](https://github.com/opensearch-project/OpenSearch/pull/18708)) - Include named queries from rescore contexts in matched_queries array ([#18697](https://github.com/opensearch-project/OpenSearch/pull/18697)) - Add the configurable limit on rule cardinality ([#18663](https://github.com/opensearch-project/OpenSearch/pull/18663)) +- Disable approximation framework when dealing with multiple sorts ([#18763](https://github.com/opensearch-project/OpenSearch/pull/18763)) ### Changed - Update Subject interface to use CheckedRunnable ([#18570](https://github.com/opensearch-project/OpenSearch/issues/18570)) diff --git a/server/src/main/java/org/opensearch/search/approximate/ApproximateMatchAllQuery.java b/server/src/main/java/org/opensearch/search/approximate/ApproximateMatchAllQuery.java index 62557bf21eee9..bcd40095dbc8f 100644 --- a/server/src/main/java/org/opensearch/search/approximate/ApproximateMatchAllQuery.java +++ b/server/src/main/java/org/opensearch/search/approximate/ApproximateMatchAllQuery.java @@ -42,6 +42,9 @@ protected boolean canApproximate(SearchContext context) { } if (context.request() != null && context.request().source() != null && context.innerHits().getInnerHits().isEmpty()) { + if (context.request().source().sorts() != null && context.request().source().sorts().size() > 1) { + return false; + } FieldSortBuilder primarySortField = FieldSortBuilder.getPrimaryFieldSortOrNull(context.request().source()); if (primarySortField != null && primarySortField.missing() == null diff --git a/server/src/main/java/org/opensearch/search/approximate/ApproximatePointRangeQuery.java b/server/src/main/java/org/opensearch/search/approximate/ApproximatePointRangeQuery.java index e6d11d02eacf6..adecb8c89ef82 100644 --- a/server/src/main/java/org/opensearch/search/approximate/ApproximatePointRangeQuery.java +++ b/server/src/main/java/org/opensearch/search/approximate/ApproximatePointRangeQuery.java @@ -447,6 +447,9 @@ public boolean canApproximate(SearchContext context) { this.setSize(Math.max(context.from() + context.size(), context.trackTotalHitsUpTo()) + 1); } if (context.request() != null && context.request().source() != null) { + if (context.request().source().sorts() != null && context.request().source().sorts().size() > 1) { + return false; + } FieldSortBuilder primarySortField = FieldSortBuilder.getPrimaryFieldSortOrNull(context.request().source()); if (primarySortField != null) { if (!primarySortField.fieldName().equals(pointRangeQuery.getField())) { From 6c502356fd4fa2d0f15df3cdc590afdf539d2c8f Mon Sep 17 00:00:00 2001 From: Prudhvi Godithi Date: Wed, 16 Jul 2025 08:38:46 -0700 Subject: [PATCH 2/3] Add tests Signed-off-by: Prudhvi Godithi --- .../ApproximatePointRangeQueryTests.java | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/server/src/test/java/org/opensearch/search/approximate/ApproximatePointRangeQueryTests.java b/server/src/test/java/org/opensearch/search/approximate/ApproximatePointRangeQueryTests.java index 1103062379a48..ae3bb2bb2b2fb 100644 --- a/server/src/test/java/org/opensearch/search/approximate/ApproximatePointRangeQueryTests.java +++ b/server/src/test/java/org/opensearch/search/approximate/ApproximatePointRangeQueryTests.java @@ -30,7 +30,10 @@ import org.apache.lucene.search.TotalHits.Relation; import org.apache.lucene.store.Directory; import org.apache.lucene.tests.index.RandomIndexWriter; +import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.search.internal.SearchContext; +import org.opensearch.search.internal.ShardSearchRequest; +import org.opensearch.search.sort.FieldSortBuilder; import org.opensearch.search.sort.SortOrder; import org.opensearch.test.OpenSearchTestCase; @@ -645,6 +648,49 @@ public void testCannotApproximateWithTrackTotalHits() { assertEquals(expectedSize, query.getSize()); } + public void testApproximateWithSort() { + long lower = RandomNumbers.randomLongBetween(random(), 0, 100); + long upper = RandomNumbers.randomLongBetween(random(), lower + 10, lower + 1000); + ApproximatePointRangeQuery query = new ApproximatePointRangeQuery( + numericType.fieldName, + numericType.encode(lower), + numericType.encode(upper), + 1, + numericType.format + ); + // Test 1: Multiple sorts should prevent approximation + { + SearchContext mockContext = mock(SearchContext.class); + ShardSearchRequest mockRequest = mock(ShardSearchRequest.class); + SearchSourceBuilder source = new SearchSourceBuilder(); + source.sort(new FieldSortBuilder(numericType.fieldName).order(SortOrder.ASC)); + source.sort(new FieldSortBuilder("another_field").order(SortOrder.ASC)); + source.terminateAfter(SearchContext.DEFAULT_TERMINATE_AFTER); + when(mockContext.aggregations()).thenReturn(null); + when(mockContext.trackTotalHitsUpTo()).thenReturn(10000); + when(mockContext.from()).thenReturn(0); + when(mockContext.size()).thenReturn(10); + when(mockContext.request()).thenReturn(mockRequest); + when(mockRequest.source()).thenReturn(source); + assertFalse("Should not approximate with multiple sorts", query.canApproximate(mockContext)); + } + // Test 2: Single sort on the same field should allow approximation + { + SearchContext mockContext = mock(SearchContext.class); + ShardSearchRequest mockRequest = mock(ShardSearchRequest.class); + SearchSourceBuilder source = new SearchSourceBuilder(); + source.sort(new FieldSortBuilder(numericType.fieldName).order(SortOrder.ASC)); + source.terminateAfter(SearchContext.DEFAULT_TERMINATE_AFTER); + when(mockContext.aggregations()).thenReturn(null); + when(mockContext.trackTotalHitsUpTo()).thenReturn(10000); + when(mockContext.from()).thenReturn(0); + when(mockContext.size()).thenReturn(10); + when(mockContext.request()).thenReturn(mockRequest); + when(mockRequest.source()).thenReturn(source); + assertTrue("Should approximate with single sort on same field", query.canApproximate(mockContext)); + } + } + // Test to cover the left child traversal in intersectRight with CELL_INSIDE_QUERY public void testIntersectRightLeftChildTraversal() throws IOException { try (Directory directory = newDirectory()) { From 22e4ae458b7665168e2b6d742e67c499cf7f30f4 Mon Sep 17 00:00:00 2001 From: Prudhvi Godithi Date: Wed, 30 Jul 2025 10:05:38 -0700 Subject: [PATCH 3/3] Fix spacing Signed-off-by: Prudhvi Godithi --- .../search/approximate/ApproximatePointRangeQueryTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/test/java/org/opensearch/search/approximate/ApproximatePointRangeQueryTests.java b/server/src/test/java/org/opensearch/search/approximate/ApproximatePointRangeQueryTests.java index 81a562dc22e7b..e7ef69b2ad8c6 100644 --- a/server/src/test/java/org/opensearch/search/approximate/ApproximatePointRangeQueryTests.java +++ b/server/src/test/java/org/opensearch/search/approximate/ApproximatePointRangeQueryTests.java @@ -1188,5 +1188,4 @@ public void testApproximateWithSort() { assertTrue("Should approximate with single sort on same field", query.canApproximate(mockContext)); } } - }