Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Case functions support: LOWER, UPPER #177

Merged
merged 13 commits into from
Sep 5, 2019
+143 −6
Merged

Case functions support: LOWER, UPPER #177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
af6938e
Integration test works
Sep 5, 2019
955feed
???
Sep 5, 2019
32d4946
All tests are working
Sep 5, 2019
0921e32
Added locale-specific test
Sep 5, 2019
6421545
Added locale-specific test
Sep 5, 2019
afac658
removed unnecessary change, fixes #128
Sep 5, 2019
4c7707f
removed unnecessary change, fixes #128
Sep 5, 2019
be6f17d
removed unnecessary change, fixes #128
Sep 5, 2019
c29f623
fixed matches, fixes #128
Sep 5, 2019
3d682c5
reverted accidentally inlined
Sep 5, 2019
98f7534
Addressed code review comments
Sep 5, 2019
32829b3
Addressed code review comments
Sep 5, 2019
77744d4
Addressed code review comments
Sep 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 67 additions & 4 deletions src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/SQLFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@
import com.google.common.collect.Sets;
import org.elasticsearch.common.collect.Tuple;

import java.util.HashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
Expand All @@ -50,7 +53,7 @@ public class SQLFunctions {
);

private static final Set<String> stringOperators = Sets.newHashSet(
"split", "concat_ws", "substring", "trim"
"split", "concat_ws", "substring", "trim", "lower", "upper"
);

private static final Set<String> binaryOperators = Sets.newHashSet(
Expand All @@ -74,6 +77,17 @@ public class SQLFunctions {
utilityFunctions)
.flatMap(Set::stream).collect(Collectors.toSet());

private Map<String, Integer> generatedIds = new HashMap<>();

/**
* Generates next id for given method name. The id's are increasing for each method name, so
* nextId("a"), nextId("a"), nextId("b") will return a_1, a_2, b_1
*/
public String nextId(String methodName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think currently, the methodName is cached in request level. If i am right, does this cached method could be shared by different request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The SqlFunctions is per-request, and this is instance field

dai-chen marked this conversation as resolved.
Show resolved Hide resolved
return methodName + "_" + generatedIds.merge(methodName, 1, Integer::sum);
}


/**
* Is the function actually translated into Elastic DSL script during execution?
*/
Expand All @@ -85,6 +99,22 @@ public Tuple<String, String> function(String methodName, List<KVValue> paramers,
boolean returnValue) {
Tuple<String, String> functionStr = null;
switch (methodName) {
case "lower": {
functionStr = lower(
(SQLExpr) paramers.get(0).value,
getLocaleForCaseChangingFunction(paramers),
name
);
break;
}
case "upper": {
functionStr = upper(
(SQLExpr) paramers.get(0).value,
getLocaleForCaseChangingFunction(paramers),
name);
break;
}

// Split is currently not supported since its using .split() in painless which is not whitelisted
case "split":
if (paramers.size() == 3) {
Expand Down Expand Up @@ -253,10 +283,36 @@ public Tuple<String, String> function(String methodName, List<KVValue> paramers,
return functionStr;
}

private int generatedId = 0;
public String getLocaleForCaseChangingFunction(List<KVValue> paramers) {
String locale;
if (paramers.size() == 1) {
locale = Locale.getDefault().getLanguage();
} else {
locale = Util.expr2Object((SQLExpr) paramers.get(1).value).toString();
}
return locale;
}

public Tuple<String, String> upper(SQLExpr field, String locale, String valueName) {
String name = nextId("upper");

public String nextId(String methodName) {
return methodName + "_" + (++generatedId);
if (valueName == null) {
return new Tuple<>(name, def(name, upper(getPropertyOrValue(field), locale)));
} else {
return new Tuple<>(name, getPropertyOrValue(field) + "; "
+ def(name, valueName + "." + upper(getPropertyOrValue(field), locale)));
}
}

public Tuple<String, String> lower(SQLExpr field, String locale, String valueName) {
String name = nextId("lower");

if (valueName == null) {
return new Tuple<>(name, def(name, lower(getPropertyOrValue(field), locale)));
} else {
return new Tuple<>(name, getPropertyOrValue(field) + "; "
+ def(name, valueName + "." + lower(getPropertyOrValue(field), locale)));
}
}

private static String def(String name, String value) {
Expand Down Expand Up @@ -527,7 +583,14 @@ public Tuple<String, String> substring(SQLExpr field, int pos, int len, String v
+ def(name, valueName + "."
+ func("substring", false, Integer.toString(pos), Integer.toString(len))));
}
}

private String lower(String property, String culture) {
return property + ".toLowerCase(Locale.forLanguageTag(\"" + culture + "\"))";
}

private String upper(String property, String culture) {
return property + ".toUpperCase(Locale.forLanguageTag(\"" + culture + "\"))";
}

/**
Expand Down
50 changes: 50 additions & 0 deletions src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/SQLFunctionsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.amazon.opendistroforelasticsearch.sql.esintgtest;


import com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils;
import org.elasticsearch.action.search.SearchResponse;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
Expand All @@ -24,12 +25,16 @@
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.search.SearchHits;
import org.hamcrest.Matcher;
import org.json.JSONArray;
import org.json.JSONObject;
import org.junit.Assert;
import org.junit.Ignore;
import org.junit.Test;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.stream.IntStream;

import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_ACCOUNT;
Expand All @@ -43,6 +48,7 @@
import static org.hamcrest.Matchers.endsWith;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasEntry;
import static org.hamcrest.Matchers.hasItems;
import static org.hamcrest.Matchers.hasValue;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.isEmptyOrNullString;
Expand Down Expand Up @@ -109,6 +115,50 @@ public void functionAlias() throws Exception {
);
}

@Test
public void caseChangeTest() throws IOException {
String query = "SELECT LOWER(firstname) " +
"FROM elasticsearch-sql_test_index_account/account " +
"WHERE UPPER(lastname)='DUKE' " +
"ORDER BY upper(lastname) ";

assertThat(
executeQuery(query),
hitAny(
kvString("/_source/address", equalTo("880 Holmes Lane")),
kvString("/fields/LOWER_1/0", equalTo("amber")))
);
}

@Test
public void caseChangeTestWithLocale() throws IOException {
// Uses Turkish locale to check if we pass correct locale for case changing functions
// "IL".toLowerCase() in a Turkish locale returns "ıl"
// https://stackoverflow.com/questions/11063102/using-locales-with-javas-tolowercase-and-touppercase

String query = "SELECT LOWER(state.keyword, 'tr') " +
"FROM elasticsearch-sql_test_index_account/account " +
"WHERE account_number=1";

assertThat(
executeQuery(query),
hitAny(
kvString("/fields/LOWER_1/0", equalTo("ıl")))
);
}

@Test
public void caseChangeWithAggregationTest() throws IOException {
String query = "SELECT UPPER(e.firstname), COUNT(*)" +
"FROM elasticsearch-sql_test_index_account/account e " +
"WHERE LOWER(e.lastname)='duke' " +
"GROUP BY UPPER(e.firstname) ";

assertThat(
executeQuery(query),
hitAny("/aggregations/UPPER_2/buckets", kvString("/key", equalTo("AMBER"))));
}

@Test
public void concat_ws_field_and_string() throws Exception {
//here is a bug,csv field with spa
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/com/amazon/opendistroforelasticsearch/sql/esintgtest/TestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,26 @@ public static String getAccountIndexMapping() {
" \"type\": \"text\",\n" +
" \"fielddata\": true\n" +
" }," +
" \"firstname\": {\n" +
" \"type\": \"text\",\n" +
" \"fielddata\": true,\n" +
" \"fields\": {\n" +
" \"keyword\": {\n" +
" \"type\": \"keyword\",\n" +
" \"ignore_above\": 256\n" +
" }" +
" }" +
" }," +
" \"lastname\": {\n" +
" \"type\": \"text\",\n" +
" \"fielddata\": true,\n" +
" \"fields\": {\n" +
" \"keyword\": {\n" +
" \"type\": \"keyword\",\n" +
" \"ignore_above\": 256\n" +
" }" +
" }" +
" }," +
" \"state\": {\n" +
" \"type\": \"text\",\n" +
" \"fielddata\": true,\n" +
Expand Down
8 changes: 6 additions & 2 deletions src/test/java/com/amazon/opendistroforelasticsearch/sql/util/MatcherUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,9 @@ public static Matcher<Map<String, Object>> kv(String key, Object value) {
return (Matcher) hasEntry(key, value);
}

public static Matcher<JSONObject> hitAny(Matcher<JSONObject>... matcher) {
public static Matcher<JSONObject> hitAny(String query, Matcher<JSONObject>... matcher) {
return featureValueOf("SearchHits", hasItems(matcher), actual -> {
JSONArray array = (JSONArray) (actual.query("/hits/hits"));
JSONArray array = (JSONArray) (actual.query(query));
List<JSONObject> results = new ArrayList<>(array.length());
for (Object element : array) {
results.add((JSONObject)element);
Expand All @@ -96,6 +96,10 @@ public static Matcher<JSONObject> hitAny(Matcher<JSONObject>... matcher) {
});
}

public static Matcher<JSONObject> hitAny(Matcher<JSONObject>... matcher) {
return hitAny("/hits/hits", matcher);
}

public static Matcher<JSONObject> hitAll(Matcher<JSONObject>... matcher) {
return featureValueOf("SearchHits", containsInAnyOrder(matcher), actual -> {
JSONArray array = (JSONArray) (actual.query("/hits/hits"));
Expand Down