Expressions with null #1946

kkondaka · 2022-10-19T18:18:01Z

Description

Added support for checking for "null" in the expressions.
With this change expressions like /response == null or /response != null can be specified as shown below

 processor:
     - grok:
        match:
          message: ['%{IPORHOST:clientip} (?:%{WORD:ident}|-) (%{USER:auth}|-) \[%{HTTPDATE:timestamp}\] "(?:%{WORD:verb} %{NOTSPACE:request}(?: HTTP/%{NUMBER:httpversion})?|%{DATA:rawrequest})" %{NUMBER:response} (?:%{NUMBER:bytes}|-) %{BASE10NUM:timetaken} "(?:%{WORD:ref}|-)" %{GREEDYDATA:useragent}']
    - drop_events:
         drop_when '/request == null'

Issues Resolved

[List any issues this PR will resolve]
#1136

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.

codecov-commenter · 2022-10-19T18:30:39Z

Codecov Report

Merging #1946 (10820ef) into main (941f808) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1946      +/-   ##
============================================
+ Coverage     93.88%   93.92%   +0.04%     
- Complexity     1536     1537       +1     
============================================
  Files           197      197              
  Lines          4561     4562       +1     
  Branches        367      367              
============================================
+ Hits           4282     4285       +3     
+ Misses          196      194       -2     
  Partials         83       83

Impacted Files	Coverage Δ
...taprepper/expression/ParseTreeCoercionService.java	`100.00% <100.00%> (ø)`
...rwarder/discovery/AwsCloudMapPeerListProvider.java	`95.12% <0.00%> (+2.43%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

dlvenable · 2022-10-19T19:55:09Z

docs/expression_syntax.md

@@ -83,6 +83,8 @@ Examples
 /status_code == 200 and /message == "Hello world"
 /status_code == 200 or /status_code == 202
 not /status_code in {200, 202}
+/response == null
+/response != null


Is there another place where we can state that null is the syntax for null checking?

I have added a statement in the "literals" section. - Null _(Supports null check to see if a Json Pointer is present or not)_. Is that not clear enough?

Added more documentation with examples.

dlvenable · 2022-10-19T19:55:45Z

...expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java

@@ -42,6 +42,8 @@ public Object coercePrimaryTerminalNode(final TerminalNode node, final Event eve
                return Float.valueOf(nodeStringValue);
            case DataPrepperExpressionParser.Boolean:
                return Boolean.valueOf(nodeStringValue);
+            case DataPrepperExpressionParser.Null:


This class has a unit test and this change should be tested there.

Yes, I thought that's what the change in this file - data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorListenerTest.java? Is there another unit test file?

Added a unit test in data-prepper-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java

dlvenable · 2022-10-19T19:57:16Z

...on/src/test/java/org/opensearch/dataprepper/expression/ConditionalExpressionEvaluatorIT.java

@@ -129,6 +129,9 @@ private static Stream<Arguments> validExpressionArguments() {
                Arguments.of("true == (/is_cool == true)", event("{\"is_cool\": true}"), true),
                Arguments.of("not /is_cool", event("{\"is_cool\": true}"), false),
                Arguments.of("/status_code < 300", event("{\"status_code\": 200}"), true),
+                Arguments.of("/status_code != null", event("{\"status_code\": 200}"), true),
+                Arguments.of("/response == null", event("{\"status_code\": 200}"), true),
+                Arguments.of("/response != null", event("{\"status_code\": 200}"), false),


Do you think there is any use in adding a test like the following?

Arguments.of("/status_code == null", event("{\"status_code\": null}"), true),

I recommend adding a few test cases with null on the left had side to test operators do not throw unexpected exceptions.

An example where a bug could occur:
Given a hypothetical operator foo that checks for class equality implemented as (lhs, rhs) -> lhs.getClass().equals(rhs.getClass())

When invoked null foo null

Then a NullPointerException will be thrown instead of true

Interesting... Is it possible to have such input? I can add the test case.

Added few test cases with null on the left side.

dlvenable · 2022-10-19T20:08:49Z

...expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java

@@ -42,6 +42,8 @@ public Object coercePrimaryTerminalNode(final TerminalNode node, final Event eve
                return Float.valueOf(nodeStringValue);
            case DataPrepperExpressionParser.Boolean:
                return Boolean.valueOf(nodeStringValue);
+            case DataPrepperExpressionParser.Null:
+                return null;


I like that this works out of box with the operators. But, should this be some sort of special class - like NullValue? I suppose this could come later if that provides specific value.

Frankly, I have no idea. I will follow what you suggest here.

It might not matter. And we can change it later since these are internal classes.

sbayer55

Thanks for adding null value support. It's cool to see Data Prepper Expressions syntax grow.

sbayer55 · 2022-10-20T17:28:26Z

...on/src/test/java/org/opensearch/dataprepper/expression/ConditionalExpressionEvaluatorIT.java

@@ -129,6 +129,9 @@ private static Stream<Arguments> validExpressionArguments() {
                Arguments.of("true == (/is_cool == true)", event("{\"is_cool\": true}"), true),
                Arguments.of("not /is_cool", event("{\"is_cool\": true}"), false),
                Arguments.of("/status_code < 300", event("{\"status_code\": 200}"), true),
+                Arguments.of("/status_code != null", event("{\"status_code\": 200}"), true),
+                Arguments.of("/response == null", event("{\"status_code\": 200}"), true),
+                Arguments.of("/response != null", event("{\"status_code\": 200}"), false),


I recommend adding a few test cases with null on the left had side to test operators do not throw unexpected exceptions.

An example where a bug could occur:
Given a hypothetical operator foo that checks for class equality implemented as (lhs, rhs) -> lhs.getClass().equals(rhs.getClass())

When invoked null foo null

Then a NullPointerException will be thrown instead of true

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

…Expressions - added more tests Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

…Expressions - updated docs Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

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

dlvenable

Thanks for expanding the conditional support with this! And thanks for making the requested test changes to help ensure quality!

kkondaka requested a review from a team as a code owner October 19, 2022 18:18

kkondaka force-pushed the expressions-with-null branch from 10820ef to 207e0a8 Compare October 19, 2022 18:41

dlvenable requested changes Oct 19, 2022

View reviewed changes

sbayer55 suggested changes Oct 20, 2022

View reviewed changes

kkondaka added 4 commits October 20, 2022 20:10

fix for issue opensearch-project#1136 Add null support to DataPrepper…

8d84213

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

fix for issue opensearch-project#1136 Add null support to DataPrepper…

8188608

…Expressions - added more tests Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

fix for issue opensearch-project#1136 Add null support to DataPrepper…

91969f7

…Expressions - updated docs Signed-off-by: Krishna Kondaka <krishkdk@amazon.com>

Addressed review comments - added test cases and updated document

ec36c7a

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

kkondaka force-pushed the expressions-with-null branch from 7e7f245 to ec36c7a Compare October 20, 2022 20:10

dlvenable approved these changes Oct 20, 2022

View reviewed changes

sbayer55 approved these changes Oct 25, 2022

View reviewed changes

dlvenable merged commit 7a6f747 into opensearch-project:main Oct 25, 2022

dlvenable mentioned this pull request Nov 14, 2022

Add null support to DataPrepperExpressions #1136

Closed

kkondaka deleted the expressions-with-null branch February 2, 2023 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expressions with null #1946

Expressions with null #1946

kkondaka commented Oct 19, 2022

codecov-commenter commented Oct 19, 2022 •

edited

Loading

dlvenable Oct 19, 2022

kkondaka Oct 20, 2022

kkondaka Oct 20, 2022

dlvenable Oct 19, 2022

kkondaka Oct 20, 2022

kkondaka Oct 20, 2022

dlvenable Oct 19, 2022

sbayer55 Oct 20, 2022

kkondaka Oct 20, 2022 •

edited

Loading

kkondaka Oct 20, 2022

sbayer55 Oct 25, 2022

dlvenable Oct 19, 2022

kkondaka Oct 20, 2022

dlvenable Oct 20, 2022

sbayer55 left a comment

sbayer55 Oct 20, 2022

dlvenable left a comment

Expressions with null #1946

Expressions with null #1946

Conversation

kkondaka commented Oct 19, 2022

Description

Issues Resolved

Check List

codecov-commenter commented Oct 19, 2022 • 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

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 Oct 20, 2022 • 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

sbayer55 left a 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

codecov-commenter commented Oct 19, 2022 •

edited

Loading

kkondaka Oct 20, 2022 •

edited

Loading