Add basic operator support to arithmetic and string expressions #2726

kkondaka · 2023-05-21T22:29:04Z

Description

Adds support for basic operators to arithmetic and string expressions.
Arithmetic operators supported are +, -, *, /

Some example arithmetic expressions supported with this change are
/key1 + /key2 +5
length(/message) - getMetadata("attr") + 10
10.2 + 20 * 6.5
(10 + 20) * 6

String operator supported + for concatenation.
Example expressions with string concatenation operator
/key1 + /key2
/message + "suffix"
getMetadata("attr") + /message

Resolves #2685 #2686

Issues Resolved

Resolves #2685 #2686

Check List

[X ] New functionality includes testing.
[X ] New functionality has been documented.
- New functionality has javadoc added
[X ] Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

graytaylor0 · 2023-05-22T16:25:15Z

data-prepper-expression/src/main/antlr/DataPrepperExpression.g4

@@ -17,16 +17,28 @@ expression
    ;

 stringExpression
-    : function
+    : stringExpression DOT stringExpression


Are we not able to reuse + for both strings and arithmetic?

…ectly Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2023-05-23T19:25:57Z

data-prepper-expression/src/main/antlr/DataPrepperExpression.g4

+    | arithmeticTerm
+    ;
+
+arithmeticTerm


Why do you call this a term?

I think we should probably find another name. A term is more generic than two numbers multiplied together.

https://simple.wikipedia.org/wiki/Term_(mathematics)

There might be better names, but perhaps you can split this into a "factor term" and a "summed term?"

dlvenable · 2023-05-23T19:27:10Z

data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/Operator.java

@@ -8,7 +8,9 @@
 import org.antlr.v4.runtime.RuleContext;

 interface Operator<T> {
-    int getNumberOfOperands();
+    default int getNumberOfOperands(final RuleContext ctx) {


Nice, I like this.

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

codecov · 2023-05-23T21:21:29Z

Codecov Report

Merging #2726 (c04dbf7) into main (f2c9341) will increase coverage by 0.50%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2726      +/-   ##
============================================
+ Coverage     93.60%   94.11%   +0.50%     
- Complexity     2270     2439     +169     
============================================
  Files           264      278      +14     
  Lines          6337     6744     +407     
  Branches        528      550      +22     
============================================
+ Hits           5932     6347     +415     
+ Misses          266      257       -9     
- Partials        139      140       +1

Impacted Files	Coverage Δ
...opensearch/dataprepper/expression/AndOperator.java	`100.00% <ø> (ø)`
...h/dataprepper/expression/GenericInSetOperator.java	`100.00% <ø> (ø)`
...aprepper/expression/GenericRegexMatchOperator.java	`100.00% <ø> (ø)`
...opensearch/dataprepper/expression/NotOperator.java	`100.00% <ø> (ø)`
...dataprepper/expression/NumericCompareOperator.java	`100.00% <ø> (ø)`
.../opensearch/dataprepper/expression/OrOperator.java	`100.00% <ø> (ø)`
.../dataprepper/model/event/DefaultEventMetadata.java	`92.00% <100.00%> (+0.33%)`	⬆️
...rch/dataprepper/event/DefaultBaseEventBuilder.java	`100.00% <100.00%> (ø)`
...arch/dataprepper/expression/AddBinaryOperator.java	`100.00% <100.00%> (ø)`
...taprepper/expression/ArithmeticBinaryOperator.java	`100.00% <100.00%> (ø)`
... and 6 more

... and 29 files with indirect coverage changes

cmanning09 · 2023-05-23T22:09:00Z

data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/BinaryOperator.java

@@ -18,11 +18,6 @@ protected BinaryOperator(final int symbol, final int shouldEvaluateRuleIndex) {
        displayName = DataPrepperExpressionParser.VOCABULARY.getDisplayName(symbol);
    }

-    @Override
-    public int getNumberOfOperands() {
-        return NUMBER_OF_OPERANDS;


Please remove line 9 in this file now.

We should leverage a single source of truth for a class. The evaluate method uses a hardcoded number to enforce the number of operands. Can we update the evaluate method to leverage the default method now as well?

cmanning09 · 2023-05-23T22:13:28Z

...er-expression/src/main/java/org/opensearch/dataprepper/expression/OperatorConfiguration.java

+                Map.of(
+                    Integer.class, (lhs, rhs) -> (double)lhs / (double)(int)rhs,
+                    Float.class, (lhs, rhs) -> (double)lhs / (Float) rhs,
+                    Long.class, (lhs, rhs) -> (double)lhs / (double)(long)rhs,


Why is the lhs cast to only a double where the rhs is cast to a long and then a double?

Because LHS is always double (final Map<Class<? extends Number>, BiFunction<Object, Object, Number>> doubleOperations) and RHS has entries for 4 different types.

cmanning09 · 2023-05-23T22:17:30Z

...test/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ArithmeticIT.java

+        if (expectedClass == null) {
+            assertThrows(ExpressionEvaluationException.class, () -> evaluator.evaluate(expression, event));
+        } else {
+            Object result = evaluator.evaluate(expression, event);
+            assertThat(result, not(instanceOf(expectedClass)));
+        }


We should separate out these tests. These are two separate behaviors that should be handled separately.

cmanning09 · 2023-05-23T22:30:03Z

docs/expression_syntax.md

+| 3     | `*`, `/`             | Multiple and Divison Operators                        | left-to-right |
 | 2     | `==`, `!=`           | Equality Operators                                    | left-to-right |
+| 2     | `+`, `-`             | Addition and Subtraction Operators                    | left-to-right |


Should we place these operations on separate levels from the the relational and equality operators? I believe we need to evaluate these operators before we can compare relataional or equality operators.

At the moment according to this documentation the following statement would result in an error with the current priority order: 2 + 4 > 5

Please confirm the priority order for these new operators and the behavior.

These changes do not support expressions like 2 + 4 > 5 for now. It is a relatively simple change from here but did not include it

While we don't support that expression type right now the documentation is miss leading and violates the standard order of operations. We need to correct this.

cmanning09 · 2023-05-23T22:35:17Z

docs/expression_syntax.md

+| `==`, `!=`           | Equality Operators            | No                   | `/status == 200`<br>`/status_code==200`                        |                                       |
+| `and`, `or`, `not`   | Conditional Operators         | Yes                  | `/a<300 and /b>200`                                            | `/b<300and/b>200`                     |
+| `,`                  | Set Value Delimiter           | No                   | `/a in {200, 202}`<br>`/a in {200,202}`<br>`/a in {200 , 202}` | `/a in {200,}`                        |
+| `+`, `-`             | Add and Subtract Operators    | No                   | `/status_code + length(/message) - 2`                          |                                       |


Should we indicate + support concatenation as well?

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable · 2023-05-25T13:20:48Z

data-prepper-expression/src/main/antlr/DataPrepperExpression.g4

+    | arithmeticFactor
+    ;
+
+arithmeticFactor


Might this be the arithmeticTerm?

Then what you have as arithmeticTerm could be an arithmeticFactor.

This page for the C grammar calls these "multiplicative_expression" and "additive_expression".

https://www.lysator.liu.se/c/ANSI-C-grammar-y.html

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable

Thanks! Looks great!

Add basic operator support to arithmetic and string expressions

1124275

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

kkondaka requested review from chenqi0805, engechas, graytaylor0, dinujoh, cmanning09, asifsmohammed, dlvenable and oeyh as code owners May 21, 2023 22:29

graytaylor0 reviewed May 22, 2023

View reviewed changes

kkondaka added 2 commits May 23, 2023 19:22

Fixed grammar to make unary and binary subtract operator to work corr…

eb370fd

…ectly Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Removed unused files

4a4f446

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

dlvenable reviewed May 23, 2023

View reviewed changes

Added tests to increase code coverage

6860fba

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

cmanning09 reviewed May 23, 2023

View reviewed changes

Addressed review comments

1b2f4b7

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

graytaylor0 previously approved these changes May 24, 2023

View reviewed changes

dlvenable reviewed May 25, 2023

View reviewed changes

Addressed review comments - Updated expression documentation

cdd6a3f

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

kkondaka dismissed graytaylor0’s stale review via cdd6a3f May 25, 2023 17:10

cmanning09 previously approved these changes May 25, 2023

View reviewed changes

Modified names in the grammar as per comments

c04dbf7

Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

kkondaka dismissed cmanning09’s stale review via c04dbf7 May 25, 2023 20:42

dlvenable approved these changes May 25, 2023

View reviewed changes

cmanning09 approved these changes May 25, 2023

View reviewed changes

This was referenced May 25, 2023

addressing build issue with s3-sink #2757

Closed

adding metric and logs in the event an S3 object does not contain records or no records were parsed from the object #2748

Merged

kkondaka merged commit 2814893 into opensearch-project:main May 25, 2023

kkondaka mentioned this pull request Jun 5, 2023

Support getting the length of a string in expressions #2639

Closed

kkondaka deleted the expr-operators branch July 13, 2023 04:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic operator support to arithmetic and string expressions #2726

Add basic operator support to arithmetic and string expressions #2726

kkondaka commented May 21, 2023 •

edited

Loading

graytaylor0 May 22, 2023

dlvenable May 23, 2023

kkondaka May 23, 2023

dlvenable May 25, 2023

dlvenable May 23, 2023

codecov bot commented May 23, 2023 •

edited

Loading

cmanning09 May 23, 2023

cmanning09 May 23, 2023

kkondaka May 24, 2023

cmanning09 May 23, 2023

cmanning09 May 23, 2023 •

edited

Loading

kkondaka May 23, 2023

cmanning09 May 24, 2023

cmanning09 May 23, 2023

dlvenable May 25, 2023

dlvenable May 25, 2023

dlvenable left a comment

Add basic operator support to arithmetic and string expressions #2726

Add basic operator support to arithmetic and string expressions #2726

Conversation

kkondaka commented May 21, 2023 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 23, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmanning09 May 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlvenable left a comment

Choose a reason for hiding this comment

kkondaka commented May 21, 2023 •

edited

Loading

codecov bot commented May 23, 2023 •

edited

Loading

cmanning09 May 23, 2023 •

edited

Loading