Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip shards when querying constant keyword fields #96161

Merged
merged 45 commits into from
Jun 9, 2023
Merged

Skip shards when querying constant keyword fields #96161

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
e036818
feature: skip shards when querying constant keyword fields
salvatore-campagna May 16, 2023
fa255a0
Update docs/changelog/96161.yaml
salvatore-campagna May 16, 2023
2f321ec
fix: use just one value for the 'area' field
salvatore-campagna May 16, 2023
1ce1c61
fix: rename class o match naming conventions
salvatore-campagna May 16, 2023
6f2dd52
fix: rename class o match naming conventions
salvatore-campagna May 16, 2023
1262e57
fix: prevent possible NPE
salvatore-campagna May 16, 2023
ea8e00d
fix: prevent possible NPE
salvatore-campagna May 16, 2023
9f9e878
test: reduce the number of shards to kick in shard pre-filtering
salvatore-campagna May 16, 2023
3805907
fix: use variable instead of raw value
salvatore-campagna May 16, 2023
12bd7c5
fix: do not try to rewrite match none queries
salvatore-campagna May 16, 2023
73fbf4c
fix: do not skip shards targeted by queries using runtime mappings
salvatore-campagna May 16, 2023
d80d825
fix: simplify logic removing MappingAwareRewriteContext
salvatore-campagna May 17, 2023
d10b7e9
fix: prevent possible NPE when searcher is null
salvatore-campagna May 17, 2023
9318d5c
fix: return intersects instead of disjoint
salvatore-campagna May 17, 2023
2fdeed1
Merge branch 'main' into feature/95541-avoid-unnecessary-search-idle-…
salvatore-campagna May 17, 2023
f998702
fix: we do not axquire the searcher if we pre-filter the shard
salvatore-campagna May 17, 2023
0de29fe
test: use a lower number of shards for shard pre-filtering
salvatore-campagna May 17, 2023
1ce266b
fix: move code such that we can reuse the index service
salvatore-campagna May 17, 2023
88a9c0f
docs: align javadoc explaining usage of null searcher
salvatore-campagna May 17, 2023
d963148
fix: move skip logic
salvatore-campagna May 17, 2023
581eede
fix: undo counter changes after moving skip logic
salvatore-campagna May 17, 2023
1acf848
fix: mve the logic in the try catch to avod leaking the searcher
salvatore-campagna May 17, 2023
65c40a4
fix: move logic before we mark the shard search active
salvatore-campagna May 17, 2023
56215c2
test: check refresh didn't happen for idle shards
salvatore-campagna May 17, 2023
f77b23f
docs: expain the idea behing skipping shards using index mappings
salvatore-campagna May 17, 2023
e654bed
fix: rename method
salvatore-campagna May 17, 2023
10d76fe
fix: restore original test name
salvatore-campagna May 17, 2023
7e84f10
fix: use multiline if statement
salvatore-campagna May 17, 2023
5981421
fix: checkstyle error on line too long
salvatore-campagna May 17, 2023
f414b5a
fix: return a boolean instead of CanMatchShardResponse
salvatore-campagna May 18, 2023
4847c08
fix: refactor some shard idle tests
salvatore-campagna May 22, 2023
75ea39f
fix: rename method argument
salvatore-campagna May 22, 2023
a0cea4e
javadoc: rephrase comment
salvatore-campagna May 22, 2023
18e4533
fix: reuse existing createIndex
salvatore-campagna May 22, 2023
e45dd7c
fix: do not check refresh stats for active shards
salvatore-campagna May 22, 2023
be1292c
Merge branch 'main' into feature/95541-avoid-unnecessary-search-idle-…
salvatore-campagna Jun 5, 2023
b5b824d
refactor: use a QueryRewriteContext instead of SearchExecutionContext
salvatore-campagna Jun 5, 2023
dfe9a6f
fix: typo in Javadoc
salvatore-campagna Jun 5, 2023
af18a36
fix: number of acquireSearchSupplier invocations
salvatore-campagna Jun 5, 2023
40140cc
fix: not using null searcher anymore
salvatore-campagna Jun 5, 2023
92d5856
docs: uodate javadocs
salvatore-campagna Jun 9, 2023
59b8445
test: can match on matching and non matching field value
salvatore-campagna Jun 9, 2023
00af0d1
fix: remove unused variable
salvatore-campagna Jun 9, 2023
b27fe03
Merge branch 'main' into feature/95541-avoid-unnecessary-search-idle-…
salvatore-campagna Jun 9, 2023
78f7ba6
fix: method parameter name
salvatore-campagna Jun 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/changelog/96161.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 96161
summary: Skip shards when querying constant keyword fields
area: "Search"
type: enhancement
issues:
- 95541
195 changes: 194 additions & 1 deletion ...asticsearch/index/shard/SearchIdleIT.java → ...icsearch/index/shard/SearchIdleTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,22 @@
import org.elasticsearch.action.get.GetRequest;
import org.elasticsearch.action.get.MultiGetRequest;
import org.elasticsearch.action.index.IndexResponse;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.index.IndexMode;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.query.ExistsQueryBuilder;
import org.elasticsearch.index.query.RangeQueryBuilder;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.test.ESSingleNodeTestCase;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.xcontent.XContentType;

import java.util.Arrays;
import java.util.List;
import java.util.Locale;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.Phaser;
Expand All @@ -33,11 +39,12 @@
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.IntToLongFunction;

import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertHitCount;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertNoSearchHits;
import static org.hamcrest.Matchers.equalTo;

public class SearchIdleIT extends ESSingleNodeTestCase {
public class SearchIdleTests extends ESSingleNodeTestCase {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this test renamed?
I think all test cases need to have the suffix IT in internalClusterTest?


public void testAutomaticRefreshSearch() throws InterruptedException {
runTestAutomaticRefresh(numDocs -> client().prepareSearch("test").get().getHits().getTotalHits().value);
Expand Down Expand Up @@ -223,4 +230,190 @@ public void testSearchIdleStats() throws InterruptedException {
assertTrue(Arrays.stream(statsResponse.getShards()).allMatch(x -> x.getSearchIdleTime() >= searchIdleAfter));
}

public void testSearchIdleBoolQueryMatchOneIndex() throws InterruptedException {
// GIVEN
final String idleIndex = "test1";
final String activeIndex = "test2";
// NOTE: we need many shards because shard pre-filtering and the "can match" phase
// are executed only if we have enough shards.
int idleIndexShardsCount = 3;
int activeIndexShardsCount = 3;
assertAcked(
client().admin()
.indices()
.prepareCreate(idleIndex)
.setSettings(
Settings.builder()
.put(IndexSettings.INDEX_SEARCH_IDLE_AFTER.getKey(), "500ms")
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, idleIndexShardsCount)
.put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), -1)
.put(IndexSettings.MODE.getKey(), IndexMode.TIME_SERIES)
.put(IndexMetadata.INDEX_ROUTING_PATH.getKey(), "routing_field")
.put(IndexSettings.TIME_SERIES_START_TIME.getKey(), "2021-05-10T00:00:00.000Z")
.put(IndexSettings.TIME_SERIES_END_TIME.getKey(), "2021-05-11T00:00:00.000Z")
)
.get()
);
assertAcked(
client().admin()
.indices()
.prepareCreate(activeIndex)
.setSettings(
Settings.builder()
.put(IndexSettings.INDEX_SEARCH_IDLE_AFTER.getKey(), "500ms")
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, activeIndexShardsCount)
.put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), -1)
.put(IndexSettings.MODE.getKey(), IndexMode.TIME_SERIES)
.put(IndexMetadata.INDEX_ROUTING_PATH.getKey(), "routing_field")
.put(IndexSettings.TIME_SERIES_START_TIME.getKey(), "2021-05-12T00:00:00.000Z")
.put(IndexSettings.TIME_SERIES_END_TIME.getKey(), "2021-05-13T23:59:59.999Z")
)
.get()
);
assertAcked(
client().admin()
.indices()
.preparePutMapping(idleIndex)
.setSource(
new String[] {
"keyword",
"type=keyword",
"@timestamp",
"type=date",
"routing_field",
"type=keyword,time_series_dimension=true" }
)
.get()
);
assertAcked(
client().admin()
.indices()
.preparePutMapping(activeIndex)
.setSource(
new String[] {
"keyword",
"type=keyword",
"@timestamp",
"type=date",
"routing_field",
"type=keyword,time_series_dimension=true" }
)
.get()
);

assertEquals(
RestStatus.CREATED,
client().prepareIndex(idleIndex)
.setSource("keyword", "idle", "@timestamp", "2021-05-10T19:00:03.765Z", "routing_field", "aaa")
.get()
.status()
);
assertEquals(
RestStatus.CREATED,
client().prepareIndex(activeIndex)
.setSource("keyword", "active", "@timestamp", "2021-05-12T20:07:12.112Z", "routing_field", "aaa")
.get()
.status()
);
assertEquals(RestStatus.OK, client().admin().indices().prepareRefresh(idleIndex, activeIndex).get().getStatus());

waitUntil(
() -> Arrays.stream(client().admin().indices().prepareStats(idleIndex, activeIndex).get().getShards())
.allMatch(ShardStats::isSearchIdle),
2,
TimeUnit.SECONDS
);

final IndicesStatsResponse statsResponse = client().admin().indices().prepareStats("test*").get();
Arrays.stream(statsResponse.getShards()).forEach(shardStats -> assertTrue(shardStats.isSearchIdle()));
Arrays.stream(statsResponse.getShards()).forEach(shardStats -> assertTrue(shardStats.getSearchIdleTime() >= 100));

// WHEN
final SearchResponse searchResponse = client().prepareSearch("test*")
.setQuery(new RangeQueryBuilder("@timestamp").from("2021-05-12T20:00:00.000Z").to("2021-05-12T21:00:00.000Z"))
.setPreFilterShardSize(5)
.get();
assertEquals(RestStatus.OK, searchResponse.status());
assertEquals(idleIndexShardsCount + activeIndexShardsCount - 1, searchResponse.getSkippedShards());
assertEquals(0, searchResponse.getFailedShards());
Arrays.stream(searchResponse.getHits().getHits()).forEach(searchHit -> assertEquals("test2", searchHit.getIndex()));
// NOTE: we need an empty result from at least one shard
assertEquals(1, searchResponse.getHits().getHits().length);

// THEN
final IndicesStatsResponse afterStatsResponse = client().admin().indices().prepareStats("test*").get();
final List<ShardStats> activeShards = Arrays.stream(afterStatsResponse.getShards()).filter(x -> x.isSearchIdle() == false).toList();
assertEquals(activeIndexShardsCount, activeShards.size());
}

public void testSearchIdleExistsQueryMatchOneIndex() throws InterruptedException {
// GIVEN
final String idleIndex = "test1";
final String activeIndex = "test2";
// NOTE: we need many shards because shard pre-filtering and the "can match" phase
// are executed only if we have enough shards.
int idleIndexShardsCount = 60;
int activeIndexShardsCount = 70;
assertAcked(
client().admin()
.indices()
.prepareCreate(idleIndex)
.setSettings(
Settings.builder()
.put(IndexSettings.INDEX_SEARCH_IDLE_AFTER.getKey(), "500ms")
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, idleIndexShardsCount)
.put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), -1)
)
.get()
);
assertAcked(
client().admin()
.indices()
.prepareCreate(activeIndex)
.setSettings(
Settings.builder()
.put(IndexSettings.INDEX_SEARCH_IDLE_AFTER.getKey(), "500ms")
.put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, activeIndexShardsCount)
.put(IndexSettings.INDEX_REFRESH_INTERVAL_SETTING.getKey(), -1)
)
.get()
);
assertAcked(client().admin().indices().preparePutMapping(idleIndex).setSource(new String[] { "keyword", "type=keyword" }).get());
assertAcked(client().admin().indices().preparePutMapping(activeIndex).setSource(new String[] { "keyword", "type=keyword" }).get());

assertEquals(RestStatus.CREATED, client().prepareIndex(idleIndex).setSource("keyword", "idle").get().status());
assertEquals(
RestStatus.CREATED,
client().prepareIndex(activeIndex).setSource("keyword", "active", "unmapped", "bbb").get().status()
);
assertEquals(RestStatus.OK, client().admin().indices().prepareRefresh(idleIndex, activeIndex).get().getStatus());

waitUntil(
() -> Arrays.stream(client().admin().indices().prepareStats(idleIndex, activeIndex).get().getShards())
.allMatch(ShardStats::isSearchIdle),
2,
TimeUnit.SECONDS
);

final IndicesStatsResponse statsResponse = client().admin().indices().prepareStats("test*").get();
Arrays.stream(statsResponse.getShards()).forEach(shardStats -> assertTrue(shardStats.isSearchIdle()));
Arrays.stream(statsResponse.getShards()).forEach(shardStats -> assertTrue(shardStats.getSearchIdleTime() >= 100));

// WHEN
final SearchResponse searchResponse = client().prepareSearch("test*")
.setQuery(new ExistsQueryBuilder("unmapped"))
.setPreFilterShardSize(5)
.get();
assertEquals(RestStatus.OK, searchResponse.status());
assertEquals(idleIndexShardsCount, searchResponse.getSkippedShards());
assertEquals(0, searchResponse.getFailedShards());
Arrays.stream(searchResponse.getHits().getHits()).forEach(searchHit -> assertEquals("test2", searchHit.getIndex()));
// NOTE: we need an empty result from at least one shard
assertEquals(1, searchResponse.getHits().getHits().length);

// THEN
final IndicesStatsResponse afterStatsResponse = client().admin().indices().prepareStats("test*").get();
final List<ShardStats> activeShards = Arrays.stream(afterStatsResponse.getShards()).filter(x -> x.isSearchIdle() == false).toList();
assertEquals(70, activeShards.size());
}
}
4 changes: 4 additions & 0 deletions server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,10 @@ public Relation isFieldWithinQuery(
DateMathParser dateParser,
QueryRewriteContext context
) throws IOException {
if (reader == null) {
// NOTE: in case we rewrite a query before we have a reader available
return Relation.INTERSECTS;
Copy link
Member

Choose a reason for hiding this comment

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

is this uncovering an existing bug that may be triggered by the percolator, or only caused by doing a rewrite before acquiring a searcher?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No bug, just doing a rewrite before we have a searcher available.

}
if (isIndexed() == false && pointsMetadataAvailable == false && hasDocValues()) {
// we don't have a quick way to run this check on doc values, so fall back to default assuming we are within bounds
return Relation.INTERSECTS;
Expand Down
16 changes: 16 additions & 0 deletions server/src/main/java/org/elasticsearch/search/SearchService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1532,6 +1532,22 @@ public CanMatchShardResponse canMatch(ShardSearchRequest request) throws IOExcep

private CanMatchShardResponse canMatch(ShardSearchRequest request, boolean checkRefreshPending) throws IOException {
assert request.searchType() == SearchType.QUERY_THEN_FETCH : "unexpected search type: " + request.searchType();
if (request.source() != null && (request.source().query() instanceof MatchNoneQueryBuilder) == false) {
final SearchExecutionContext searchExecutionContext = new SearchExecutionContext(
indicesService.indexServiceSafe(request.shardId().getIndex())
jimczi marked this conversation as resolved.
Show resolved Hide resolved
.newSearchExecutionContext(
request.shardId().getId(),
request.shardRequestIndex(),
null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Using null here is okish but we should update the javadoc in SearchExecutionContext?

request::nowInMillis,
request.getClusterAlias(),
request.getRuntimeMappings()
)
);
if (queryStillMatchesAfterRewrite(request, searchExecutionContext) == false) {
return new CanMatchShardResponse(false, null);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The MappingAwareRewriteContext doesn't seem to be needed. Why not creating a new SearchExecutionContext when the index service is available.
You should also reuse queryStillMatchesAfterRewrite(request, context) to ensure that all cases are checked (alias filter and global aggregations).

Copy link
Member

Choose a reason for hiding this comment

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

The MappingAwareRewriteContext doesn't seem to be needed. Why not creating a new SearchExecutionContext when the index service is available.

If we use SearchExecutionContext then also range queries (and maybe others?) could be resolved to MatchNoneQueryBuilder instance. But if there are pending refreshes we can't trust it. The idea was to have a special context that only rewrites with the mappings in mind (for constant keyword fields). If the main query gets rewritten to MatchNoneQueryBuilder then we can trust it even when there are pending refreshes.

You should also reuse queryStillMatchesAfterRewrite(request, context) to ensure that all cases are checked (alias filter and global aggregations).

Good point. This should be possible even with MappingAwareRewriteContext .

Copy link
Contributor

Choose a reason for hiding this comment

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

If we use SearchExecutionContext then also range queries (and maybe others?) could be resolved to MatchNoneQueryBuilder instance.

The initial change already used SearchExecutionContext but it sets the searcher to null. That's why I said that the special context is not needed. There's some logic to handle SearchExecutionContext with a null searcher, we should add some javadocs to explain the use case (rewriting with the mapping only) but it works.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I see this now. Yes, if we pass a null searcher then this should work. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had to add an additional null check in isFieldWithinQuery. That logic uses the reader which might be null in this case and triggers a NPE at some point.

Releasable releasable = null;
try {
IndexService indexService;
Expand Down
Loading