Skip to content

Conversation

@original-brownbear
Copy link
Contributor

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
Contributor 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
Contributor 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
Contributor 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