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.

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

Script: Optimize for the expected non-intersecting case #93279

Closed

Conversation

joegallo
Copy link
Contributor

It's not expected that there are overlapping keys so optimize for the expected case -- we only actually need the intersection to build a friendly error message anyway.

It's not expected that there are overlapping keys so optimize for the
expected case -- we only actually need the intersection to build a
friendly error message anyway.
@joegallo joegallo added >enhancement :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache Team:Core/Infra Meta label for core/infra team v8.7.0 labels Jan 26, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine
Copy link
Collaborator

Hi @joegallo, I've created a changelog YAML for you.

@joegallo
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/part-1

/**
* Compare two sets and return true if they have any overlapping keys. (That is, if there's an intersection.)
*/
private static boolean hasOverlappingKeys(Set<String> set1, Set<String> set2) {
Copy link
Member

Choose a reason for hiding this comment

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

This method is almost exactly what Sets.intersection does. It's going to be linear on the smaller set size. The only difference I see is in the common case creating a zero length set. Why is that zero length set a problem (something that we shouldn't create)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, that's a really good point! Closing this in favor of #93284, that PR rewrites Sets.intersection to be faster, and so our calling it here is no longer expensive enough to warrant avoiding.

@joegallo
Copy link
Contributor Author

Closing this in favor of #93284

@joegallo joegallo closed this Jan 26, 2023
@joegallo joegallo deleted the ingest-ctx-map-set-intersection branch January 26, 2023 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache >enhancement Team:Core/Infra Meta label for core/infra team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants