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

Conversation

original-brownbear
Copy link
Member

It's in the title, saving some allocations and weird indirection in obvious spots.

It's in the title, saving some allocations and weird indirection in obvious spots.
@original-brownbear original-brownbear added >non-issue :Data Management/Indices APIs APIs to create and manage indices and templates labels Apr 23, 2025
@elasticsearchmachine elasticsearchmachine added v9.1.0 Team:Data Management Meta label for data/management team labels Apr 23, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

Copy link
Contributor

@nielsbauman nielsbauman left a comment

Choose a reason for hiding this comment

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

I added two questions purely for my understanding, but LGTM. Thanks Armin!

Comment on lines -377 to +384
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)
))) {
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 :)

Comment on lines +1765 to +1775
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();
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 :)

@original-brownbear
Copy link
Member Author

Thanks Niels!

@original-brownbear original-brownbear merged commit e8e068b into elastic:main Apr 23, 2025
18 checks passed
@original-brownbear original-brownbear deleted the fix-parsing-aliasfilter branch April 23, 2025 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport pending :Data Management/Indices APIs APIs to create and manage indices and templates >non-issue Team:Data Management Meta label for data/management team v8.19.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants