Optimize the canMatch phase on the data node #14511

bugmakerrrrrr · 2024-06-23T08:11:29Z

Description

Two changes here:

short circuit can match check, respond with CanMatchResponse(false, null) if cannot match(minMax value is useless if corresponding shard is not match);
only primary searchAfter field will be evaluated if exists.

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 · 2024-06-23T09:00:18Z

✅ Gradle check result for ada899d: SUCCESS

codecov · 2024-06-23T09:01:08Z

Codecov Report

Attention: Patch coverage is 60.60606% with 13 lines in your changes missing coverage. Please review.

Project coverage is 71.72%. Comparing base (49b7cd4) to head (8c91a12).
Report is 199 commits behind head on main.

Files with missing lines	Patch %	Lines
...main/java/org/opensearch/search/SearchService.java	75.00%	0 Missing and 6 partials ⚠️
...ensearch/search/internal/ContextIndexSearcher.java	0.00%	4 Missing and 1 partial ⚠️
...nsearch/search/searchafter/SearchAfterBuilder.java	33.33%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #14511      +/-   ##
============================================
- Coverage     71.84%   71.72%   -0.12%     
+ Complexity    62820    62720     -100     
============================================
  Files          5169     5169              
  Lines        294664   294672       +8     
  Branches      42615    42621       +6     
============================================
- Hits         211691   211344     -347     
- Misses        65530    65898     +368     
+ Partials      17443    17430      -13

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jainankitk

@bugmakerrrrrr - Thank you for this PR. Given the single threaded nature of canMatch phase, every optimization is critical, especially when 100-1000s of shards are involved! Is there any way to quantify the impact of this change? Also, are the existing tests sufficient to catch any regressions? If not, can we add test cases for the missing code paths?

jainankitk · 2024-06-26T22:30:27Z

server/src/main/java/org/opensearch/search/SearchService.java

+                if (aliasFilterCanMatch == false) {
+                    return new CanMatchResponse(false, null);
+                }
+
+                // null query means match_all
+                boolean canMatch = canRewriteToMatchNone(request.source()) == false
+                    || request.source().query() instanceof MatchNoneQueryBuilder == false;
+                if (canMatch == false) {
+                    return new CanMatchResponse(false, null);
                }


Can this be better written as below:

if (aliasFilterCanMatch == false || (canRewriteToMatchNone(request.source()) && request.source().query() instanceof MatchNoneQueryBuilder)) return new CanMatchResponse(false, null); }

jainankitk · 2024-06-26T22:35:50Z

server/src/main/java/org/opensearch/search/SearchService.java

+    private static FieldDoc getPrimarySearchAfterFieldDoc(
+        FieldSortBuilder primarySortBuilder,
+        Object primarySearchAfter,
+        QueryShardContext context
+    ) throws IOException {
+        final Optional<SortAndFormats> sortOpt = SortBuilder.buildSort(List.of(primarySortBuilder), context);
+        return sortOpt.map(sortAndFormats -> SearchAfterBuilder.buildFieldDoc(sortAndFormats, new Object[] { primarySearchAfter }))
+            .orElse(null);


Unable to see any optimization from this refactoring. Seems more like restructuring and not sure if the readability is improving.

The previous implementation would construct all sortFields if user request contains multi SortBuilders and search after fields, but what we need is just the primary sort search after field.

Is it possible to qualitatively quantify the impact? Maybe some approximation of the worst case query earlier that would be improved with this change

I think that this is quite simple, for example, if I have following query:

{ "query": { "match_all": {} }, "track_total_hits": false, "sort": [ { "grade": "desc" }, { "age": "asc" }, { "id": "asc" } ], "search_after": [ 10, 20, 12345 ] }

Previously, we have to evaluate and build these all three sort fields, now we just need to build the primary sort field. The idea behind this change is same as FieldSortBuilder.getPrimaryFieldSortOrNull.

jainankitk · 2024-06-26T23:06:40Z

server/src/main/java/org/opensearch/search/SearchService.java

+                boolean canMatch = canRewriteToMatchNone(request.source()) == false
+                    || request.source().query() instanceof MatchNoneQueryBuilder == false;
+                if (canMatch == false) {
+                    return new CanMatchResponse(false, null);


Can we assume that CanMatchResponse(false, null) equivalent to CanMatchResponse(false,<non-null>)?

I think yes, CanMatchPreFilterSearchPhase extract the min/max values of each shard response and use them to pre-sort the shards prior to running the subsequent phases. However, the unmatched shard will be skipped in the following phases, so its min/max value will have no impact on the query task.

bugmakerrrrrr · 2024-06-29T15:20:11Z

Is there any way to quantify the impact of this change? Also, are the existing tests sufficient to catch any regressions?

No, this optimization is just for reducing unnecessary computation or IO. Currently, I am unable to accurately measure the impact of this change.

If not, can we add test cases for the missing code paths?

Sure.

github-actions · 2024-06-29T17:17:54Z

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

bugmakerrrrrr · 2024-07-10T15:12:01Z

@jainankitk friendly ping

jainankitk · 2024-07-16T00:37:12Z

server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java

+        if (searchContext.searchAfter() != null
+            && searchContext.request() != null
+            && searchContext.request().source() != null
+            && Objects.equals(searchContext.trackTotalHitsUpTo(), SearchContext.TRACK_TOTAL_HITS_DISABLED)) {


Any specific reason for using Object.equals given both the values are primitives

No, I just copied the code from SearchService (in which trackTotalHitsUpto can be null), I can fix this.

Can you update this?

jainankitk · 2024-07-16T19:56:23Z

server/src/main/java/org/opensearch/search/SearchService.java

+    private static FieldDoc getPrimarySearchAfterFieldDoc(
+        FieldSortBuilder primarySortBuilder,
+        Object primarySearchAfter,
+        QueryShardContext context
+    ) throws IOException {
+        final Optional<SortAndFormats> sortOpt = SortBuilder.buildSort(List.of(primarySortBuilder), context);
+        return sortOpt.map(sortAndFormats -> SearchAfterBuilder.buildFieldDoc(sortAndFormats, new Object[] { primarySearchAfter }))
+            .orElse(null);


Is it possible to qualitatively quantify the impact? Maybe some approximation of the worst case query earlier that would be improved with this change

jainankitk · 2024-07-16T19:58:06Z

@bugmakerrrrrr - While the changes look okay, I am still unsure about the impact and need of this code change. Even if not using some benchmark, having some idea of potential improvement with this change will be good!

bugmakerrrrrr · 2024-07-17T12:31:45Z

server/src/main/java/org/opensearch/search/internal/ContextIndexSearcher.java

-        if (searchContext.searchAfter() != null && searchContext.request() != null && searchContext.request().source() != null) {
+        // Skipping search on shard/segment entirely can cause mismatch on total_tracking_hits, hence skip only if
+        // track_total_hits is false.
+        if (searchContext.searchAfter() != null


@jainankitk The potential improvements are mostly related to short circuit. For example, we can check the conditions upfront here and avoid to build sort field or read min max values unnecessary.

bugmakerrrrrr · 2024-07-17T12:33:11Z

server/src/main/java/org/opensearch/search/SearchService.java

-                } else {
-                    // null query means match_all
-                    canMatch = aliasFilterCanMatch;
+                if (aliasFilterCanMatch == false


here is the same

jainankitk · 2024-08-05T16:18:27Z

server/src/main/java/org/opensearch/search/SearchService.java

+
+                if (hasRefreshPending) {
+                    final FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source());
+                    final MinAndMax<?> minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null;
+                    return new CanMatchResponse(true, minMax);
+                }
+
                final boolean aliasFilterCanMatch = request.getAliasFilter().getQueryBuilder() instanceof MatchNoneQueryBuilder == false;
-                FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source());
-                MinAndMax<?> minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null;
-                boolean canMatch;
-                if (canRewriteToMatchNone(request.source())) {
-                    QueryBuilder queryBuilder = request.source().query();
-                    canMatch = aliasFilterCanMatch && queryBuilder instanceof MatchNoneQueryBuilder == false;
-                } else {
-                    // null query means match_all
-                    canMatch = aliasFilterCanMatch;
+                if (aliasFilterCanMatch == false


Can we rewrite this as below:

aliasFilterCanMatch is used as double negation, better change it to aliasFilterCannotMatch

sortBuilder and minMax can be done in one place if CanMatch(false, null) is pulled up!

if (!hasRefreshPending) { final boolean aliasFilterCannotMatch = request.getAliasFilter().getQueryBuilder() instanceof MatchNoneQueryBuilder; if (aliasFilterCannotMatch || (canRewriteToMatchNone(request.source()) && request.source().query() instanceof MatchNoneQueryBuilder) { return new CanMatchResponse(false, null); } } final FieldSortBuilder sortBuilder = FieldSortBuilder.getPrimaryFieldSortOrNull(request.source()); final MinAndMax<?> minMax = sortBuilder != null ? FieldSortBuilder.getMinMaxOrNull(context, sortBuilder) : null; if (hasRefreshPending) { return new CanMatchResponse(true, minMax); }

jainankitk

Thanks @bugmakerrrrrr, LGTM except one minor comment!

jainankitk · 2024-08-05T16:54:54Z

Also can you add entry in CHANGELOG.md, before this change can be reviewed by maintainer and merged!

jainankitk · 2024-08-05T17:23:57Z

server/src/main/java/org/opensearch/search/SearchService.java

+                        canMatch = canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, trackTotalHitsUpto);
+                    }
+                }
+                return new CanMatchResponse(canMatch, canMatch ? minMax : null);


Also, can we just not send over minMax? Should get ignored if canMatch is false anyway?

I think that setting minMax as null is equivalent to not sending this value.

Then we probably should not even care what we are sending for minMax if canMatch is false? So below should also work?

new CanMatchResponse(canMatch, minMax);

jainankitk · 2024-08-05T17:25:25Z

server/src/main/java/org/opensearch/search/SearchService.java

+                // Check for sort.missing == null, since in case of missing values sort queries, if segment/shard's min/max
+                // is out of search_after range, it still should be printed and hence we should not skip segment/shard.
+                if (Objects.equals(trackTotalHitsUpto, SearchContext.TRACK_TOTAL_HITS_DISABLED)
+                    && minMax != null


Do we need minMax != null here, since it is validated in canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, trackTotalHitsUpto)?

if minMax == null, we can avoid to build primary sort field. The validation in canMatchSearchAfter is mainly used in query phase.

Signed-off-by: panguixin <[email protected]>

github-actions · 2024-08-06T13:45:41Z

✅ Gradle check result for 8c91a12: SUCCESS

jainankitk

LGTM! Can one of the maintainers help review this PR? @msfroh @sohami @reta

jainankitk · 2024-08-07T20:23:37Z

server/src/main/java/org/opensearch/search/SearchService.java

+                        canMatch = canMatchSearchAfter(searchAfterFieldDoc, minMax, sortBuilder, trackTotalHitsUpto);
+                    }
+                }
+                return new CanMatchResponse(canMatch, canMatch ? minMax : null);


Then we probably should not even care what we are sending for minMax if canMatch is false? So below should also work?

new CanMatchResponse(canMatch, minMax);

jainankitk · 2024-08-07T20:26:20Z

@bugmakerrrrrr - Can you resolve the failing CHANGELOG verifier and codecov check while one of the maintainer reviews this PR?

opensearch-trigger-bot · 2024-09-07T15:21:48Z

This PR is stalled because it has been open for 30 days with no activity.

bugmakerrrrrr requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, dbwiddis, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159, sachinpkale, saratvemulapalli, shwetathareja, sohami and VachaShah as code owners June 23, 2024 08:11

mch2 assigned jainankitk Jun 26, 2024

jainankitk reviewed Jun 26, 2024

View reviewed changes

bugmakerrrrrr force-pushed the can_match branch from ada899d to 5186664 Compare June 29, 2024 16:30

bugmakerrrrrr requested a review from ashking94 as a code owner June 29, 2024 16:30

This was referenced Jun 29, 2024

[AUTOCUT] Gradle Check Flaky Test Report for RemoteCloneIndexIT #14292

Open

[AUTOCUT] Gradle Check Flaky Test Report for RemoteStoreStatsIT #14310

Open

jainankitk reviewed Jul 16, 2024

View reviewed changes

bugmakerrrrrr commented Jul 17, 2024

View reviewed changes

jainankitk reviewed Aug 5, 2024

View reviewed changes

jainankitk approved these changes Aug 5, 2024

View reviewed changes

jainankitk reviewed Aug 5, 2024

View reviewed changes

bugmakerrrrrr added 3 commits August 6, 2024 19:38

optimize the canMatch phase on the data node

15d55bc

Signed-off-by: panguixin <[email protected]>

refactor and add test

27a1324

Signed-off-by: panguixin <[email protected]>

apply review comments

8c91a12

Signed-off-by: panguixin <[email protected]>

bugmakerrrrrr force-pushed the can_match branch from 5186664 to 8c91a12 Compare August 6, 2024 12:52

jainankitk approved these changes Aug 7, 2024

View reviewed changes

opensearch-trigger-bot bot added the stalled Issues that have stalled label Sep 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize the canMatch phase on the data node #14511

Optimize the canMatch phase on the data node #14511

bugmakerrrrrr commented Jun 23, 2024

github-actions bot commented Jun 23, 2024

codecov bot commented Jun 23, 2024 •

edited

Loading

jainankitk left a comment

jainankitk Jun 26, 2024

jainankitk Jun 26, 2024

bugmakerrrrrr Jun 29, 2024

jainankitk Jul 16, 2024

bugmakerrrrrr Jul 17, 2024

jainankitk Jun 26, 2024

bugmakerrrrrr Jun 29, 2024

bugmakerrrrrr commented Jun 29, 2024

github-actions bot commented Jun 29, 2024

bugmakerrrrrr commented Jul 10, 2024

jainankitk Jul 16, 2024

bugmakerrrrrr Jul 17, 2024

jainankitk Aug 5, 2024

bugmakerrrrrr Aug 6, 2024

jainankitk Jul 16, 2024

jainankitk commented Jul 16, 2024

bugmakerrrrrr Jul 17, 2024

bugmakerrrrrr Jul 17, 2024

jainankitk Aug 5, 2024 •

edited

Loading

jainankitk left a comment

jainankitk commented Aug 5, 2024

jainankitk Aug 5, 2024

bugmakerrrrrr Aug 6, 2024

jainankitk Aug 7, 2024

jainankitk Aug 5, 2024

bugmakerrrrrr Aug 6, 2024

github-actions bot commented Aug 6, 2024

jainankitk left a comment •

edited

Loading

jainankitk Aug 7, 2024

jainankitk commented Aug 7, 2024

opensearch-trigger-bot bot commented Sep 7, 2024

Optimize the canMatch phase on the data node #14511

Are you sure you want to change the base?

Optimize the canMatch phase on the data node #14511

Conversation

bugmakerrrrrr commented Jun 23, 2024

Description

Related Issues

Check List

github-actions bot commented Jun 23, 2024

codecov bot commented Jun 23, 2024 • edited Loading

Codecov Report

jainankitk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bugmakerrrrrr commented Jun 29, 2024

github-actions bot commented Jun 29, 2024

bugmakerrrrrr commented Jul 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainankitk commented Jul 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainankitk Aug 5, 2024 • edited Loading

Choose a reason for hiding this comment

jainankitk left a comment

Choose a reason for hiding this comment

jainankitk commented Aug 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 6, 2024

jainankitk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jainankitk commented Aug 7, 2024

opensearch-trigger-bot bot commented Sep 7, 2024

codecov bot commented Jun 23, 2024 •

edited

Loading

jainankitk Aug 5, 2024 •

edited

Loading

jainankitk left a comment •

edited

Loading