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

Ignore case when parsing `script_values_sorted|unique` in aggregations. #5010

Closed
wants to merge 1 commit into from

Conversation

@jpountz
Copy link
Contributor

commented Feb 4, 2014

Close #5009

@s1monw
s1monw reviewed Feb 6, 2014
View changes
src/main/java/org/elasticsearch/common/Strings.java Outdated
/**
* Compare two strings for equality, regardless of whether they are written in camel case or underscore case.
*/
public static boolean equalsIgnoreCase(String s1, String s2) {

This comment has been minimized.

Copy link
@s1monw

s1monw Feb 6, 2014

Contributor

Maybe you should use CharSequence here instead as the param type?

@s1monw
s1monw reviewed Feb 6, 2014
View changes
src/test/java/org/elasticsearch/common/StringsTests.java Outdated
@Test
public void testEqualsIgnoreCase() {
final String s = randomRealisticUnicodeOfCodepointLength(10);
assertEqualsIgnoreCase(s, s);

This comment has been minimized.

Copy link
@s1monw

s1monw Feb 6, 2014

Contributor

add assertEqualsIgnoreCase(s, new String(s)); :)

This comment has been minimized.

Copy link
@s1monw

s1monw Feb 6, 2014

Contributor

also a loop with

final String s = randomRealisticUnicodeOfCodepointLength(10);
assertEqualsIgnoreCase(s, s.toUpperCase(Locale.ROOT));

maybe?

@s1monw
s1monw reviewed Feb 6, 2014
View changes
src/main/java/org/elasticsearch/search/aggregations/metrics/valuecount/ValueCountParser.java Outdated
@@ -69,7 +70,7 @@ public AggregatorFactory parse(String aggregationName, XContentParser parser, Se
throw new SearchParseException(context, "Unknown key for a " + token + " in [" + aggregationName + "]: [" + currentFieldName + "].");
}
} else if (token == XContentParser.Token.VALUE_BOOLEAN) {
if ("script_values_unique".equals(currentFieldName)) {
if (Strings.equalsIgnoreCase("script_values_unique", currentFieldName)) {

This comment has been minimized.

Copy link
@s1monw

s1monw Feb 6, 2014

Contributor

hmm general question, why do we not use "script_values_unique".equalsIgnoreCase(currentFieldName)

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2014

LGTM

@jpountz jpountz closed this Feb 6, 2014
@jpountz jpountz deleted the jpountz:fix/aggregations-camelcase branch Feb 6, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.