[PPL] Support expression and boolean operators #518
[PPL] Support expression and boolean operators #518
Conversation
Corrected column names for functions/operators; Added unit tests for <, <=, >, >=.
Added integration test cases.
String result = | ||
executeQueryToString( | ||
String.format("source=%s | stats avg(abs(age)*2) as AGE", TEST_INDEX_ACCOUNT)); | ||
assertEquals( |
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.
when should we use plan text comparsion? when shoud with use matchers like verifyColumn?
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.
To my understanding, we can use verifyColumn like matchers in most of time, but some cases that some other content other than columns and datarows are also important to veriy, so we can flexibly choose text comparison. For example, to verify the formats, or verify the explain results etc.
As for the case this comment referred to, I would like to verify schema, column names, result set of the aggregation, so I chose to compare text.
integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/OperatorIT.java
Show resolved
Hide resolved
integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/OperatorIT.java
Show resolved
Hide resolved
integ-test/src/test/java/com/amazon/opendistroforelasticsearch/sql/ppl/QueryAnalysisIT.java
Show resolved
Hide resolved
...on/opendistroforelasticsearch/sql/expression/operator/predicate/BinaryPredicateOperator.java
Outdated
Show resolved
Hide resolved
@@ -210,8 +260,85 @@ private static FunctionResolver equal() { | |||
); | |||
} | |||
|
|||
private static Map<FunctionSignature, FunctionBuilder> predicateFunction( | |||
private static FunctionResolver notEqual() { |
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.
could we define notEqual as the combination of not(equal)
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.
I was also thinking about it didn't figure it out yet
core/src/main/java/com/amazon/opendistroforelasticsearch/sql/data/model/ExprValue.java
Outdated
Show resolved
Hide resolved
General suggestion, if the PR related to different issue or subject, we should submit serval small PR accordingly instead of using large PR. |
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.
Thanks for the change.
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.
LGTM. Thanks!
Issue #, if available:
Description of changes:
1. Support Boolean operators
Boolean operators supported in the pull request include:
Additionally supplemented operators in analyzer:
2. Corrected column name of nested functions/operators (related: #517)
Example:
source=accounts | stats avg(abs(age))
Before: returned column name was in pattern
avg(object@#)
After: column name is
avg(abs(age))
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.