Add support for basic arithmetic and string returning expressions to DataPrepper Expression #2697

kkondaka · 2023-05-15T23:28:44Z

Description

Add support for basic arithmetic and string returning expressions to DataPrepper Expression.

Adds support for expressions returning arithmetic (integer/float) values from function/JsonPointers/literals

Also, adds support for expressions returning string values from function/JsonPointers/literals

Partially addresses the issues #2686 and #2685
Resolves #2703

Issues Resolved

#2703

Check List

[X ] New functionality includes testing.
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.

codecov · 2023-05-16T00:32:26Z

Codecov Report

Merging #2697 (08c40a6) into main (a540932) will increase coverage by 0.00%.
The diff coverage is 90.90%.

@@            Coverage Diff            @@
##               main    #2697   +/-   ##
=========================================
  Coverage     93.61%   93.61%           
- Complexity     2267     2269    +2     
=========================================
  Files           263      264    +1     
  Lines          6330     6331    +1     
  Branches        526      526           
=========================================
+ Hits           5926     5927    +1     
  Misses          266      266           
  Partials        138      138

Impacted Files	Coverage Δ
...h/dataprepper/pipeline/router/RouterAppConfig.java	`0.00% <ø> (ø)`
...lugins/processor/parsejson/ParseJsonProcessor.java	`86.88% <0.00%> (ø)`
...ch/dataprepper/expression/ExpressionEvaluator.java	`100.00% <100.00%> (ø)`
...taprepper/pipeline/router/RouteEventEvaluator.java	`100.00% <100.00%> (ø)`
...rch/dataprepper/pipeline/router/RouterFactory.java	`100.00% <100.00%> (ø)`
...prepper/expression/GenericExpressionEvaluator.java	`100.00% <100.00%> (ø)`
...rch/dataprepper/expression/ParseTreeEvaluator.java	`100.00% <100.00%> (ø)`

…l types of expressions Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

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

graytaylor0 · 2023-05-17T17:57:23Z

...prepper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java

+    @Test
+    public void testDefaultEvaluateConditionalThrows() {
+        expressionEvaluator = new TestExpressionEvaluator();
+        assertThrows(ClassCastException.class, () -> expressionEvaluator.evaluateConditional("/status", event("{\"status\":200}")));


Not related to this PR, but I don't love how a conditional expression of /status throws this Exception. I would prefer /status to check if the key exists, and /status == true to actually evaluate if the value is true.

This raises an interesting point which I didn't consider.

By combining the expression evaluator, there really is no way to evaluate one expression with some conditional goal in mind versus evaluating with an object in mind.

I don't think that should hinder this PR because I think it could be resolved with some internal refactoring.

We already have a way to check for existence /status == null would check for key exists or not. As David said, that functionality is not new.

Yes that does work, I'm just saying I think /status as an alternative to /status != null is cleaner.

graytaylor0 · 2023-05-17T18:03:14Z

...ion/src/test/java/org/opensearch/dataprepper/expression/ArithmeticExpressionEvaluatorIT.java

+import static org.hamcrest.CoreMatchers.not;
+import org.apache.commons.lang3.RandomStringUtils;
+
+class ArithmeticExpressionEvaluatorIT {


Since there is no ArithmeticExpressionEvaluator class, is this really an IT for GenericExpressionEvaluator?

Another approach that is fairly common in the Java world is to split tests using an underscore. This is testing the GenericExpressionEvaluator for arithmetic and you have another testing GenericExpressionEvaluator for strings.

Thus:

GenericExpressionEvaluator_ArithmeticIT

GenericExpressionEvaluator_StringIT

This has the benefit of putting them next to each other in file tree UIs.

I'm fine keeping what you have or using this pattern.

Will rename to GenericExpressionEvaluator_ArithmeticIT and GenericExpressionEvaluator_StringIT

dlvenable · 2023-05-17T18:38:27Z

...prepper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java

+
+public class ExpressionEvaluatorTest {
+    private ExpressionEvaluator expressionEvaluator;
+    class TestExpressionEvaluator implements ExpressionEvaluator {


You could use a mock and tell it to use real methods for the default method. But, it's ok this way too. I'm just pointing out that possibility.

dlvenable · 2023-05-17T18:41:56Z

...prepper-api/src/test/java/org/opensearch/dataprepper/expression/ExpressionEvaluatorTest.java

+    @Test
+    public void testDefaultEvaluateConditionalThrows() {
+        expressionEvaluator = new TestExpressionEvaluator();
+        assertThrows(ClassCastException.class, () -> expressionEvaluator.evaluateConditional("/status", event("{\"status\":200}")));


This raises an interesting point which I didn't consider.

By combining the expression evaluator, there really is no way to evaluate one expression with some conditional goal in mind versus evaluating with an object in mind.

I don't think that should hinder this PR because I think it could be resolved with some internal refactoring.

dlvenable · 2023-05-17T18:43:41Z

...epper-expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeEvaluator.java

        try {
            final ParseTreeEvaluatorListener listener = new ParseTreeEvaluatorListener(operatorProvider, coercionService, event);
            walker.walk(listener, parseTree);
-            return coercionService.coerce(listener.getResult(), Boolean.class);
+            return listener.getResult();


Are there any possible limitations here? Could this break some existing scenarios?

I can't think of cases where it can break the existing scenarios. All the existing scenarios are changed to use Boolean and they all check for return Object type to be Boolean and if not throw an error.

dlvenable · 2023-05-17T18:45:44Z

...ion/src/test/java/org/opensearch/dataprepper/expression/ArithmeticExpressionEvaluatorIT.java

+import static org.hamcrest.CoreMatchers.not;
+import org.apache.commons.lang3.RandomStringUtils;
+
+class ArithmeticExpressionEvaluatorIT {


Another approach that is fairly common in the Java world is to split tests using an underscore. This is testing the GenericExpressionEvaluator for arithmetic and you have another testing GenericExpressionEvaluator for strings.

Thus:

GenericExpressionEvaluator_ArithmeticIT

GenericExpressionEvaluator_StringIT

This has the benefit of putting them next to each other in file tree UIs.

I'm fine keeping what you have or using this pattern.

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

…pper into arith-expr

dlvenable · 2023-05-17T20:45:46Z

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

+        if (result instanceof Boolean) {
+            return (Boolean) result;
+        } else {
+            throw new ClassCastException("Unexpected expression return type of " + result.getClass());


I took a look at the ParseTreeCoercionService class. It appears that it will throw the following when it fails to get the correct value.

throw new ExpressionCoercionException("Unable to cast " + obj.getClass().getName() + " into " + clazz.getName());

Perhaps, we should move this evaluateConditional implementation into the GenericExpressionEvaluator and use that ParseTreeCoercionService class instead.

So this would be an unimplemented method here in the interface.

evaluateConditional(String statement, Event context);

Then the GenericExpressionEvaluator looks somewhat like:

@Override public Boolean evaluateConditional(final String statement, final Event context) { final Object result = evaluate(statement, context); return coersionService.coerce(result, Boolean.class); }

@dlvenable Yes, I could change ClassCastException to ExpressionCoercionException if that helps. Ultimately there is not much difference between the two when the type is Boolean because Boolean cannot be Coerced to a different type anyways. I don't see the need to move the evaluateConditional to GenericExpressionEvaluator just for that purpose because Boolean is not really coercion friendly anyways. Let me know if you strongly prefer calling coersionService here.

kkondaka requested review from chenqi0805, engechas, graytaylor0, dinujoh, cmanning09, asifsmohammed, dlvenable and oeyh as code owners May 15, 2023 23:28

Modified to create GenericExpressionEvaluator that can be used for al…

5bb2481

…l types of expressions Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

kkondaka force-pushed the arith-expr branch from 720a425 to 5bb2481 Compare May 17, 2023 00:24

Added ExpressionEvaluatorTest.java

18d8d9a

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

graytaylor0 reviewed May 17, 2023

View reviewed changes

dlvenable requested changes May 17, 2023

View reviewed changes

Renamed test files as per review comments

407f216

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

graytaylor0 approved these changes May 17, 2023

View reviewed changes

Merge branch 'main' of https://github.com/opensearch-project/data-pre…

08c40a6

…pper into arith-expr

dlvenable requested changes May 17, 2023

View reviewed changes

dlvenable approved these changes May 19, 2023

View reviewed changes

dlvenable merged commit f2c9341 into opensearch-project:main May 19, 2023

kkondaka deleted the arith-expr branch July 13, 2023 04:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for basic arithmetic and string returning expressions to DataPrepper Expression #2697

Add support for basic arithmetic and string returning expressions to DataPrepper Expression #2697

kkondaka commented May 15, 2023 •

edited

Loading

codecov bot commented May 16, 2023 •

edited

Loading

graytaylor0 May 17, 2023 •

edited

Loading

dlvenable May 17, 2023

kkondaka May 17, 2023

graytaylor0 May 17, 2023

graytaylor0 May 17, 2023

dlvenable May 17, 2023

kkondaka May 17, 2023 •

edited

Loading

dlvenable May 17, 2023

dlvenable May 17, 2023

dlvenable May 17, 2023

kkondaka May 17, 2023

dlvenable May 17, 2023

dlvenable May 17, 2023

kkondaka May 18, 2023 •

edited

Loading

Add support for basic arithmetic and string returning expressions to DataPrepper Expression #2697

Add support for basic arithmetic and string returning expressions to DataPrepper Expression #2697

Conversation

kkondaka commented May 15, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented May 16, 2023 • edited Loading

Codecov Report

graytaylor0 May 17, 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

kkondaka May 17, 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

Choose a reason for hiding this comment

kkondaka May 18, 2023 • edited Loading

Choose a reason for hiding this comment

kkondaka commented May 15, 2023 •

edited

Loading

codecov bot commented May 16, 2023 •

edited

Loading

graytaylor0 May 17, 2023 •

edited

Loading

kkondaka May 17, 2023 •

edited

Loading

kkondaka May 18, 2023 •

edited

Loading