Adding simple In support #1114
Adding simple In support #1114
Conversation
…is returning float, and some tests for the new engine were disabled for that
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
I think it's fine to remove this file if the code base has no dependency on it.
| } else if (o instanceof ExprValue) { | ||
| // since there is no primitive in Java for differentiating TIMESTAMP DATETIME and DATE | ||
| // we can allow passing a ExprValue that already contains this information | ||
| return (ExprValue) o; |
There was a problem hiding this comment.
I'll wait for a few more comment/suggestions before making changes to the rest.
Thank you @chloe-zh for the comments!
| // disabling test because in case of expr collections, we could pass ExprValues | ||
| // @Test | ||
| // public void unSupportedObject() { | ||
| // Exception exception = assertThrows(ExpressionEvaluationException.class, | ||
| // () -> ExprValueUtils.fromObjectValue(integerValue(1))); | ||
| // assertEquals( | ||
| // "unsupported object " | ||
| // + "class com.amazon.opendistroforelasticsearch.sql.data.model.ExprIntegerValue", | ||
| // exception.getMessage()); | ||
| // } | ||
|
|
| JSONObject results = executeQuery(sql); | ||
| Assert.assertThat(getTotalHits(results), equalTo(1)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could you add some NOT IN integration test cases here?
| @@ -370,3 +371,11 @@ functionArg | |||
| : expression | |||
| ; | |||
|
|
|||
| arrayArgs | |||
There was a problem hiding this comment.
I think it should be collection of expression instead of array. Here are the reference from bigquery.
There was a problem hiding this comment.
is it a naming change or a structural change?
Would renaming arrayArgs to expressionCollection be the only thing to change?
There was a problem hiding this comment.
This is the current limitation with the Function/Operator definition and resolver which doesn't support VAR paramaters. Most of the implementation is pretty good, my only concern is it is not ARRAY data type. Looking for the solution to support VAR paramaters now.
There was a problem hiding this comment.
Could you please add some details on next steps?
From my understanding you suggest to discard this PR away and add implementation for VARs.
- Is this correct?
- is there any plan/timeline on your side?
Thank you,
Francesco
There was a problem hiding this comment.
The ARRAY is also make sense, but the defintion of IN operator should be IN(BOOL, ARRAY(BOOL)). which required more change in Type (support ARRAY(E)), FunctionSignature.
Add my thoughts in here. Does it make sense to you?
This works for
field IN (list...)The support to subqueries in the IN clause is yet to come.