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

Skip sorting in ArrayTagSet for SortedMaps #999

Merged
merged 1 commit into from Oct 14, 2022

Conversation

kilink
Copy link
Member

@kilink kilink commented Oct 14, 2022

When tags are supplied in a SortedMap, skip the sorting step. Additionally, skip deduping when tags are supplied in a Map in general.

@brharrington brharrington added this to the 1.3.10 milestone Oct 14, 2022
checkForNullValues(newTags);
return addAll(newTags, newTags.length);

boolean sorted = ts instanceof SortedMap && ((SortedMap<?, ?>) ts).comparator() == null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also check if the natural ordering is explicit?

private boolean hasNaturalOrder(SortedMap<String, String> ts) {
  return ts.comparator() == null || ts.comparator() == Comparator.<String>naturalOrder();
}

That would cover usage like new TreeMap<>(Comparator.naturalOrder()), though not new TreeMap<>(String::compareTo).

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about checking for Comparator.naturalOrder(), although I think it might be unusual to explicitly supply that comparator since the Javadoc for SortedMap / TreeMap indicate natural ordering is the default. I can go ahead and add a check for that anyway, but I think it's not possible to cover all scenarios (Guava collections use an explicit comparator for natural ordering that is different than Comparator.naturalOrder(), and String::compareTo like you mentioned.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I've updated the PR to check for Comparator.naturalOrder() as well.

When tags are supplied in a SortedMap, skip the sorting step. Additionally, skip
deduping when tags are supplied in a Map in general.
@brharrington brharrington merged commit 7955006 into Netflix:main Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants