Skip to content

Some obvious speedups to parsing alias filters #127240

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.

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -374,11 +374,14 @@ protected static Collection<ResolvedExpression> resolveExpressionsToResources(Co
return List.of();
}
} else {
Predicate<String> isMatchAll = (((Predicate<String>) Metadata.ALL::equals)).or(Regex::isMatchAllPattern);
if (expressions == null
|| expressions.length == 0
|| expressions.length == 1
&& (SelectorResolver.selectorsValidatedAndMatchesPredicate(expressions[0], context, isMatchAll))) {
&& (SelectorResolver.selectorsValidatedAndMatchesPredicate(
expressions[0],
context,
s -> Metadata.ALL.equals(s) || Regex.isMatchAllPattern(s)
))) {
Comment on lines -377 to +384
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, hat's the value of inlining this variable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Metadata.all::equals and the .or call both actually allocate an object so this removes allocation outright (as well as some overhead for the method handles).
Also, the .or doesn't compile anything really, it just chains two predicates generically, unless that inlines we're just burning extra cycles for method calls vs a non-capturing lambda :)

IndexComponentSelector selector;
if (expressions != null && expressions.length == 1) {
selector = SelectorResolver.parseMatchAllToSelector(context, expressions[0]);
Expand Down
64 changes: 28 additions & 36 deletions server/src/main/java/org/elasticsearch/indices/IndicesService.java
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,12 @@ public IndicesQueryCache getIndicesQueryCache() {
return indicesQueryCache;
}

private QueryBuilder parseFilter(BytesReference bytes) throws IOException {
try (XContentParser parser = XContentHelper.createParser(parserConfig, bytes)) {
return parseTopLevelQuery(parser);
}
}

static class OldShardsStats implements IndexEventListener {

final SearchStats searchStats = new SearchStats();
Expand Down Expand Up @@ -1743,13 +1749,6 @@ interface IndexDeletionAllowedPredicate {
public AliasFilter buildAliasFilter(ProjectState project, String index, Set<ResolvedExpression> resolvedExpressions) {
/* Being static, parseAliasFilter doesn't have access to whatever guts it needs to parse a query. Instead of passing in a bunch
* of dependencies we pass in a function that can perform the parsing. */
CheckedFunction<BytesReference, QueryBuilder, IOException> filterParser = bytes -> {
try (
XContentParser parser = XContentHelper.createParserNotCompressed(parserConfig, bytes, XContentHelper.xContentType(bytes))
) {
return parseTopLevelQuery(parser);
}
};

final ProjectMetadata metadata = project.metadata();
String[] aliases = indexNameExpressionResolver.filteringAliases(metadata, index, resolvedExpressions);
Expand All @@ -1759,43 +1758,36 @@ public AliasFilter buildAliasFilter(ProjectState project, String index, Set<Reso

IndexAbstraction ia = metadata.getIndicesLookup().get(index);
DataStream dataStream = ia.getParentDataStream();
final QueryBuilder filter;
if (dataStream != null) {
var dsAliases = metadata.dataStreamAliases();
String dataStreamName = dataStream.getName();
List<QueryBuilder> filters = Arrays.stream(aliases)
.map(name -> metadata.dataStreamAliases().get(name))
.filter(dataStreamAlias -> dataStreamAlias.getFilter(dataStreamName) != null)
.map(dataStreamAlias -> {
try {
return filterParser.apply(dataStreamAlias.getFilter(dataStreamName).uncompressed());
} catch (IOException e) {
throw new UncheckedIOException(e);
}
})
.toList();
List<QueryBuilder> filters = Arrays.stream(aliases).map(key -> {
var f = dsAliases.get(key).getFilter(dataStreamName);
if (f == null) {
return null;
}
try {
return parseFilter(f.compressedReference());
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}).filter(Objects::nonNull).toList();
Comment on lines +1765 to +1775
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the value of reordering these stream operations?

Copy link
Member Author

Choose a reason for hiding this comment

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

We save one lookup on the aliases map by not doing an extra one just to null check :)

if (filters.isEmpty()) {
return AliasFilter.of(null, aliases);
filter = null;
} else if (filters.size() == 1) {
filter = filters.getFirst();
} else {
if (filters.size() == 1) {
return AliasFilter.of(filters.get(0), aliases);
} else {
BoolQueryBuilder bool = new BoolQueryBuilder();
for (QueryBuilder filter : filters) {
bool.should(filter);
}
return AliasFilter.of(bool, aliases);
BoolQueryBuilder bool = new BoolQueryBuilder();
for (QueryBuilder f : filters) {
bool.should(f);
}
filter = bool;
}
} else {
IndexMetadata indexMetadata = metadata.index(index);
return AliasFilter.of(ShardSearchRequest.parseAliasFilter(filterParser, indexMetadata, aliases), aliases);
filter = ShardSearchRequest.parseAliasFilter(this::parseFilter, metadata.index(index), aliases);
}
}

/**
* Returns a new {@link QueryRewriteContext} with the given {@code now} provider
*/
public QueryRewriteContext getRewriteContext(LongSupplier nowInMillis, ResolvedIndices resolvedIndices, PointInTimeBuilder pit) {
return getRewriteContext(nowInMillis, resolvedIndices, pit, false);
return AliasFilter.of(filter, aliases);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.function.Function;

import static java.util.Collections.emptyMap;
import static org.elasticsearch.search.internal.SearchContext.TRACK_TOTAL_HITS_DISABLED;
Expand Down Expand Up @@ -630,23 +629,13 @@ public static QueryBuilder parseAliasFilter(
}
Index index = metadata.getIndex();
Map<String, AliasMetadata> aliases = metadata.getAliases();
Function<AliasMetadata, QueryBuilder> parserFunction = (alias) -> {
if (alias.filter() == null) {
return null;
}
try {
return filterParser.apply(alias.filter().uncompressed());
} catch (IOException ex) {
throw new AliasFilterParsingException(index, alias.getAlias(), "Invalid alias filter", ex);
}
};
if (aliasNames.length == 1) {
AliasMetadata alias = aliases.get(aliasNames[0]);
if (alias == null) {
// This shouldn't happen unless alias disappeared after filteringAliases was called.
throw new InvalidAliasNameException(index, aliasNames[0], "Unknown alias name was passed to alias Filter");
}
return parserFunction.apply(alias);
return parseAliasFilter(filterParser, alias, index);
} else {
// we need to bench here a bit, to see maybe it makes sense to use OrFilter
BoolQueryBuilder combined = new BoolQueryBuilder();
Expand All @@ -656,7 +645,7 @@ public static QueryBuilder parseAliasFilter(
// This shouldn't happen unless alias disappeared after filteringAliases was called.
throw new InvalidAliasNameException(index, aliasNames[0], "Unknown alias name was passed to alias Filter");
}
QueryBuilder parsedFilter = parserFunction.apply(alias);
QueryBuilder parsedFilter = parseAliasFilter(filterParser, alias, index);
if (parsedFilter != null) {
combined.should(parsedFilter);
} else {
Expand All @@ -668,6 +657,21 @@ public static QueryBuilder parseAliasFilter(
}
}

private static QueryBuilder parseAliasFilter(
CheckedFunction<BytesReference, QueryBuilder, IOException> filterParser,
AliasMetadata alias,
Index index
) {
if (alias.filter() == null) {
return null;
}
try {
return filterParser.apply(alias.filter().compressedReference());
} catch (IOException ex) {
throw new AliasFilterParsingException(index, alias.getAlias(), "Invalid alias filter", ex);
}
}

public final Map<String, Object> getRuntimeMappings() {
return source == null ? emptyMap() : source.runtimeMappings();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.UUIDs;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.compress.CompressorFactory;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.settings.Settings;
Expand Down Expand Up @@ -210,7 +211,7 @@ private IndexMetadata add(IndexMetadata indexMetadata, String alias, @Nullable C
public QueryBuilder aliasFilter(IndexMetadata indexMetadata, String... aliasNames) {
return ShardSearchRequest.parseAliasFilter(bytes -> {
try (
InputStream inputStream = bytes.streamInput();
InputStream inputStream = CompressorFactory.COMPRESSOR.uncompress(bytes).streamInput();
XContentParser parser = XContentFactory.xContentType(inputStream)
.xContent()
.createParser(xContentRegistry(), DeprecationHandler.THROW_UNSUPPORTED_OPERATION, inputStream)
Expand Down