-
Notifications
You must be signed in to change notification settings - Fork 151
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
[Calcite Engine] Support In expression #3429
Changes from 7 commits
8c17ba0
4e00944
7ef4897
67a65af
24c5a9e
1bfa823
e480134
1714f78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,12 +9,25 @@ | |
import org.junit.Ignore; | ||
import org.opensearch.sql.ppl.WhereCommandIT; | ||
|
||
@Ignore("Not all boolean functions are supported in Calcite now") | ||
public class CalciteWhereCommandIT extends WhereCommandIT { | ||
@Override | ||
public void init() throws IOException { | ||
enableCalcite(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we disableCalcite also? otherwise it may impact other tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have |
||
disallowCalciteFallback(); | ||
super.init(); | ||
} | ||
|
||
@Ignore("https://github.com/opensearch-project/sql/issues/3428") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #3428 is LIKE function, why releated to testIsNotNull? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this test case contains a like function as well.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
@Override | ||
public void testIsNotNullFunction() throws IOException {} | ||
|
||
@Ignore("https://github.com/opensearch-project/sql/issues/3333") | ||
@Override | ||
public void testWhereWithMetadataFields() throws IOException {} | ||
|
||
@Override | ||
protected String getIncompatibleTypeErrMsg() { | ||
return "In expression types are incompatible: fields type BIGINT, values type [INTEGER," | ||
+ " INTEGER, CHAR(4)]"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CHAR(4) is not a valid OpenSearch PPL datatype. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,12 +5,14 @@ | |
|
||
package org.opensearch.sql.ppl; | ||
|
||
import static org.hamcrest.CoreMatchers.containsString; | ||
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_ACCOUNT; | ||
import static org.opensearch.sql.legacy.TestsConstants.TEST_INDEX_BANK_WITH_NULL_VALUES; | ||
import static org.opensearch.sql.util.MatcherUtils.rows; | ||
import static org.opensearch.sql.util.MatcherUtils.verifyDataRows; | ||
|
||
import java.io.IOException; | ||
import org.hamcrest.MatcherAssert; | ||
import org.json.JSONObject; | ||
import org.junit.jupiter.api.Test; | ||
|
||
|
@@ -106,4 +108,73 @@ public void testWhereWithMetadataFields() throws IOException { | |
String.format("source=%s | where _id='1' | fields firstname", TEST_INDEX_ACCOUNT)); | ||
verifyDataRows(result, rows("Amber")); | ||
} | ||
|
||
@Test | ||
public void testWhereWithIn() throws IOException { | ||
JSONObject result = | ||
executeQuery( | ||
String.format( | ||
"source=%s | where firstname in ('Amber') | fields firstname", TEST_INDEX_ACCOUNT)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. need a test for
|
||
verifyDataRows(result, rows("Amber")); | ||
|
||
result = | ||
executeQuery( | ||
String.format( | ||
"source=%s | where firstname in ('Amber', 'Dale') | fields firstname", | ||
TEST_INDEX_ACCOUNT)); | ||
verifyDataRows(result, rows("Amber"), rows("Dale")); | ||
|
||
result = | ||
executeQuery( | ||
String.format( | ||
"source=%s | where balance in (4180, 5686.0) | fields balance", | ||
TEST_INDEX_ACCOUNT)); | ||
verifyDataRows(result, rows(4180), rows(5686)); | ||
} | ||
|
||
@Test | ||
public void testWhereWithNotIn() throws IOException { | ||
JSONObject result = | ||
executeQuery( | ||
String.format( | ||
"source=%s | where account_number < 4 | where firstname not in ('Amber', 'Levine')" | ||
+ " | fields firstname", | ||
TEST_INDEX_ACCOUNT)); | ||
verifyDataRows(result, rows("Roberta"), rows("Bradshaw")); | ||
|
||
result = | ||
executeQuery( | ||
String.format( | ||
"source=%s | where account_number < 4 | where not firstname in ('Amber', 'Levine')" | ||
+ " | fields firstname", | ||
TEST_INDEX_ACCOUNT)); | ||
verifyDataRows(result, rows("Roberta"), rows("Bradshaw")); | ||
|
||
result = | ||
executeQuery( | ||
String.format( | ||
"source=%s | where not firstname not in ('Amber', 'Dale') | fields firstname", | ||
TEST_INDEX_ACCOUNT)); | ||
verifyDataRows(result, rows("Amber"), rows("Dale")); | ||
} | ||
|
||
@Test | ||
public void testInWithIncompatibleType() { | ||
Exception e = | ||
assertThrows( | ||
Exception.class, | ||
() -> { | ||
executeQuery( | ||
String.format( | ||
"source=%s | where balance in (4180, 5686, '6077') | fields firstname", | ||
TEST_INDEX_ACCOUNT)); | ||
}); | ||
MatcherAssert.assertThat(e.getMessage(), containsString(getIncompatibleTypeErrMsg())); | ||
} | ||
|
||
protected String getIncompatibleTypeErrMsg() { | ||
return "function expected" | ||
+ " {[BYTE,BYTE],[SHORT,SHORT],[INTEGER,INTEGER],[LONG,LONG],[FLOAT,FLOAT],[DOUBLE,DOUBLE],[STRING,STRING],[BOOLEAN,BOOLEAN],[DATE,DATE],[TIME,TIME],[TIMESTAMP,TIMESTAMP],[INTERVAL,INTERVAL],[IP,IP],[STRUCT,STRUCT],[ARRAY,ARRAY]}," | ||
+ " but got [LONG,STRING]"; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,6 +154,11 @@ public void testNotExpression() { | |
assertEquals("source=t | where not a = ***", anonymize("source=t | where not a=1 ")); | ||
} | ||
|
||
@Test | ||
public void testInExpression() { | ||
assertEquals("source=t | where a in (***)", anonymize("source=t | where a in (1, 2, 3) ")); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a test for
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "not in" is not supported in our g4, which mean v2 engine don't support it either. Need to add that implementation for v2 engine as well. |
||
} | ||
|
||
@Test | ||
public void testQualifiedName() { | ||
assertEquals("source=t | fields + field0", anonymize("source=t | fields field0")); | ||
|
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.
Calcite will optimize In expression to equal comparison if there is only 1 element in value list. We don't need to do such optimization ourself here like v2.
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.
Do u mean we push down IN to OpenSearch as terms query, so Calcite optimized is not needed?