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

Function support for IS/IS NOT in WHERE clause #344

Closed
wants to merge 4 commits into from
Closed

Function support for IS/IS NOT in WHERE clause #344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
34e5fe6
Function support for IS/IS NOT in WHERE clause
davidcui1225 Jan 13, 2020
b49119c
addressed comments about IT format
davidcui1225 Jan 13, 2020
5df3248
Merge remote-tracking branch 'upstream/master' into issue-341
davidcui1225 Jan 15, 2020
2cf7ba4
Merge remote-tracking branch 'upstream/master' into issue-341
davidcui1225 Jan 22, 2020
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
4 changes: 3 additions & 1 deletion src/main/java/com/amazon/opendistroforelasticsearch/sql/parser/WhereParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -576,8 +576,10 @@ private SQLMethodInvokeExpr parseSQLBinaryOpExprWhoIsConditionInWhere(SQLBinaryO

String operator = soExpr.getOperator().getName();

if (operator.equals("=")) {
if (operator.equals("=") || operator.equals("IS")) {
operator = "==";
} else if (operator.equals("IS NOT")) {
davidcui1225 marked this conversation as resolved.
Show resolved Hide resolved
operator = "!=";
}

String finalStr = v1Dec + v2Dec + v1 + " " + operator + " " + v2;
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/com/amazon/opendistroforelasticsearch/sql/utils/Util.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ public static Object expr2Object(SQLExpr expr, String charWithQuote) {
value = expr.toString();
} else if (expr instanceof SQLAllColumnExpr) {
value = "*";
} else if (expr instanceof SQLNullExpr) {
value = "\"NULL\"";
} else if (expr instanceof SQLValuableExpr) {
value = ((SQLValuableExpr) expr).getValue();
} else {
Expand Down
67 changes: 67 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 @@ -34,6 +34,7 @@
import java.util.stream.IntStream;

import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_ACCOUNT;
import static com.amazon.opendistroforelasticsearch.sql.esintgtest.TestsConstants.TEST_INDEX_EMPLOYEE_NESTED;
import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.hitAny;
import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.kvDouble;
import static com.amazon.opendistroforelasticsearch.sql.util.MatcherUtils.kvInt;
Expand All @@ -60,6 +61,7 @@ public class SQLFunctionsIT extends SQLIntegTestCase {
@Override
protected void init() throws Exception {
loadIndex(Index.ACCOUNT);
loadIndex(Index.EMPLOYEE_NESTED);
}

@Test
Expand Down Expand Up @@ -316,6 +318,71 @@ public void castIntFieldToDoubleWithAliasJdbcFormatGroupByTest() {
}
}

@Test
public void functionAbsInWhereClauseWithIsNullTest() {
JSONObject response = executeJdbcRequest(
"SELECT age FROM " + TEST_INDEX_ACCOUNT + " WHERE (ABS(balance) IS NULL)"
Copy link
Member

Choose a reason for hiding this comment

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

So all your IT use TEST_INDEX_ACCOUNT index for testing, I wonder is there any document in this index missing balance or firstname field? Because I recall the painless scripts we generate are unable to handle missing field (#308), I assume your IT actually doesn't cover the typical use of IS NULL or IS NOT NULL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point- I'll look for/create an index with some missing field values

Copy link
Contributor

Choose a reason for hiding this comment

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

One suggestion, instead of create new index, could we consider using the existing index. I think there are already 10+ different indices in place.

);

assertThat(response.query("/schema/0/name"), equalTo("age"));
assertThat(response.query("/schema/0/type"), equalTo("long"));

Assert.assertThat(response.getJSONArray("datarows").length(), equalTo(0));
}

@Test
public void functionAbsInWhereClauseWithIsNotNullTest() {
JSONObject response = executeJdbcRequest(
"SELECT name FROM " + TEST_INDEX_EMPLOYEE_NESTED +
" WHERE (UPPER(title) IS NULL)"
);

assertThat(response.query("/schema/0/name"), equalTo("name"));
assertThat(response.query("/schema/0/type"), equalTo("text"));

// Integer[] expectedOutput = new Integer[] {40, 39, 38, 37, 36};
System.out.println(response.getJSONArray("datarows").length());
for (int i = 0; i < response.getJSONArray("datarows").length(); ++i) {
// Assert.assertThat(
// response.getJSONArray("datarows")
// .getJSONArray(i).get(0),
// equalTo(expectedOutput[i]));
System.out.println(response.getJSONArray("datarows").getJSONArray(i).getString(0));
Comment on lines +346 to +350
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove it.

}
}

@Test
public void functionUpperInWhereClauseWithIsNullTest() {
JSONObject response = executeJdbcRequest(
"SELECT age FROM " + TEST_INDEX_ACCOUNT + " WHERE (UPPER(firstname) IS NULL)"
);

assertThat(response.query("/schema/0/name"), equalTo("age"));
assertThat(response.query("/schema/0/type"), equalTo("long"));

Assert.assertThat(response.getJSONArray("datarows").length(), equalTo(0));
}

@Test
public void functionUpperInWhereClauseWithIsNotNullTest() {
JSONObject response = executeJdbcRequest(
"SELECT DISTINCT age FROM " + TEST_INDEX_ACCOUNT +
" WHERE (UPPER(firstname) IS NOT NULL) ORDER BY age ASC LIMIT 5"
);

assertThat(response.query("/schema/0/name"), equalTo("age"));
assertThat(response.query("/schema/0/type"), equalTo("long"));

Integer[] expectedOutput = new Integer[] {20, 21, 22, 23, 24};
for (int i = 0; i < response.getJSONArray("datarows").length(); ++i) {
Assert.assertThat(
response.getJSONArray("datarows")
.getJSONArray(i).get(0),
equalTo(expectedOutput[i]));
}
}


@Test
public void concat_ws_field_and_string() throws Exception {
//here is a bug,csv field with spa
Expand Down