-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Conversation
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.
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Hi @joegallo, I've created a changelog YAML for you. |
@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) { |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
Closing this in favor of #93284 |
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.