ADD: ParseTreeCoercionService and tests #1153

chenqi0805 · 2022-03-08T16:42:15Z

Description

As a breakdown of #1145 , this PR adds ParseTreeCoercionService that is responsible for type checking and casting on both terminal node in the ParseTree and evaluation result.

Issues Resolved

Contributes to #1003

Check List

New functionality includes testing.
New functionality has been documented.
- New functionality has javadoc added
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: Chen <[email protected]>

sbayer55

Thanks @chenqi0805 for breaking up the larger PR. Overall everything is looking great. I added a few comments, the only requested change is enhancing the testCoerceTerminalNodeEscapedJsonPointerType() test case.

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

sbayer55 · 2022-03-08T16:52:06Z

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

+
+@Named
+class ParseTreeCoercionService {
+    public Object coerceTerminalNode(final TerminalNode node, final Event event) throws ParseTreeCoercionException {


It looks like this method will only handle terminal nodes within a primary/literal context. If primary terminal nodes are the only use case, consider a more descriptive name. One suggestion I have is coercePrimaryNode.

@sbayer55, would that new name suggest this method supports setInitializer and variableIdentifier as defined in the grammar language for primary? Are you also suggesting we are missing some coercion functionality or do we just need a better name?

primary : jsonPointer | variableIdentifier | setInitializer | literal ;

I'm suggesting if a better name is possible. Considering operator tokens (==, in ...) are TerminalNodes but this method could not coerce them.

...ession/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java

sbayer55 · 2022-03-08T16:58:33Z

...ession/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java

+    }
+
+    @Test
+    void testCoerceTerminalNodeEscapedJsonPointerType() throws ParseTreeCoercionException {


Thanks for explicitly testing JsonPointer and EscapedJsonPointer! Within testCoerceTerminalNodeEscapedJsonPointerType() consider using special characters in your json pointer path. Some of the trickier characters are ~, \, /, and .

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

dlvenable · 2022-03-08T19:52:09Z

...pression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionException.java

+ * @since 1.3
+ * Exception thrown by {@link ParseTreeCoercionService} methods.
+ */
+public class ParseTreeCoercionException extends Exception {


From a caller's perspective, the parse tree doesn't mean much. I think we could use a better name here. Perhaps ExpressionCoercionException.

dlvenable · 2022-03-08T19:53:49Z

...pression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionException.java

+
+/**
+ * @since 1.3
+ * Exception thrown by {@link ParseTreeCoercionService} methods.


In line with my other comment on this exception, please update the Javadocs to explain what this means to the caller/client.

dlvenable · 2022-03-08T19:54:51Z

...ession/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java

+        assertThrows(ParseTreeCoercionException.class, () -> objectUnderTest.coerce(testObj, String.class));
+    }
+
+    private Event createTestEvent(final Object data) {


You could decouple your tests from JacksonEvent and probably simplify these tests by using a mock. But, I can accept the current code.

Turns out I do have to mock Event in order to test tricky characters in escape jsonpointer which is not currently supported by JacksonEvent.

dlvenable · 2022-03-08T19:55:47Z

...ession/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java

+    @Test
+    void testCoerceTerminalNodeIntegerType() throws ParseTreeCoercionException {
+        when(token.getType()).thenReturn(DataPrepperExpressionParser.Integer);
+        final Integer testInteger = 1;


I strongly recommend the use of random numbers here (within a decent range). It is quite easy to write against a test with a value like 1.

Alternatively, you can make this a @ParameterizedTest and verify against a few values.

Good suggestiong @dlvenable!

As a best practice, to guarantee consistent test results, I recommend avoiding random values in test cases. Either method is acceptable in this use case and both are used throught Data Preppers test code.

dlvenable · 2022-03-08T19:56:02Z

...ession/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java

+    @Test
+    void testCoerceTerminalNodeFloatType() throws ParseTreeCoercionException {
+        when(token.getType()).thenReturn(DataPrepperExpressionParser.Float);
+        final Float testFloat = 1.0f;


Again, please do not test against 1.0.

dlvenable · 2022-03-08T19:58:41Z

...ession/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java

+    @Test
+    void testCoerceFailure() {
+        final Object testObj = false;
+        assertThrows(ParseTreeCoercionException.class, () -> objectUnderTest.coerce(testObj, String.class));


This test might be more readable if you provided a mock(Object.class) instead of false. Presently, it seems that false means something specific, but you are just creating a different type.

Unfortunately one can not mock getClass method since it is final. I will use TestObject here to indicate the difference.

Signed-off-by: Chen <[email protected]>

codecov-commenter · 2022-03-09T17:31:51Z

Codecov Report

Merging #1153 (18cef17) into main (818d9cb) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #1153   +/-   ##
=========================================
  Coverage     91.89%   91.89%           
  Complexity      703      703           
=========================================
  Files            87       87           
  Lines          2037     2037           
  Branches        171      171           
=========================================
  Hits           1872     1872           
  Misses          122      122           
  Partials         43       43

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 818d9cb...18cef17. Read the comment docs.

sbayer55 · 2022-03-09T17:38:07Z

...ession/src/test/java/org/opensearch/dataprepper/expression/ParseTreeCoercionServiceTest.java

+    }
+
+    @ParameterizedTest
+    @MethodSource("provideKeys")


Thanks for adding this!

sbayer55 · 2022-03-09T17:42:53Z

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

+        final String nodeStringValue = node.getText();
+        switch (nodeType) {
+            case DataPrepperExpressionParser.EscapedJsonPointer:
+                return event.get(nodeStringValue.substring(1, nodeStringValue.length() - 1), Object.class);


nit: reading the code here I understand what is happening but not why. It might be a good idea to include a variable or create a private function.

final String jsonPointerWithoutQuotes = nodeStringValue.substring(1, nodeStringValue.length() - 1); // or private String removeQuotesFrom(final String text) { return nodeStringValue.substring(1, nodeStringValue.length() - 1) }

Signed-off-by: Chen <[email protected]>

ADD: ParseTreeCoercionService and tests

62b2f21

Signed-off-by: Chen <[email protected]>

chenqi0805 requested review from dlvenable and sbayer55 March 8, 2022 16:42

chenqi0805 requested a review from a team as a code owner March 8, 2022 16:42

sbayer55 suggested changes Mar 8, 2022

View reviewed changes

dlvenable requested changes Mar 8, 2022

View reviewed changes

chenqi0805 added 5 commits March 8, 2022 16:55

MNT: reuse nodeStringValue

f3edf68

Signed-off-by: Chen <[email protected]>

MAINT: address PR comments

abd17a4

Signed-off-by: Chen <[email protected]>

MAINT: test case name

0723ae7

Signed-off-by: Chen <[email protected]>

TST: parameterized tests on escape json pointer

e0a30a4

Signed-off-by: Chen <[email protected]>

MAINT: fix test cases

18cef17

Signed-off-by: Chen <[email protected]>

chenqi0805 requested review from cmanning09, sbayer55 and dlvenable March 9, 2022 17:24

sbayer55 previously approved these changes Mar 9, 2022

View reviewed changes

MNT: separate variable

40712f0

Signed-off-by: Chen <[email protected]>

chenqi0805 dismissed sbayer55’s stale review via 40712f0 March 9, 2022 17:48

chenqi0805 requested a review from sbayer55 March 9, 2022 17:48

sbayer55 approved these changes Mar 9, 2022

View reviewed changes

cmanning09 approved these changes Mar 9, 2022

View reviewed changes

dlvenable approved these changes Mar 9, 2022

View reviewed changes

chenqi0805 merged commit 7cf318a into opensearch-project:main Mar 9, 2022

chenqi0805 deleted the feat/1003-parse-tree-coercion-service branch March 9, 2022 19:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADD: ParseTreeCoercionService and tests #1153

ADD: ParseTreeCoercionService and tests #1153

chenqi0805 commented Mar 8, 2022 •

edited

Loading

sbayer55 left a comment

sbayer55 Mar 8, 2022

cmanning09 Mar 8, 2022 •

edited

Loading

sbayer55 Mar 8, 2022

sbayer55 Mar 8, 2022

dlvenable Mar 8, 2022

dlvenable Mar 8, 2022

dlvenable Mar 8, 2022

chenqi0805 Mar 9, 2022

dlvenable Mar 8, 2022

sbayer55 Mar 8, 2022

dlvenable Mar 8, 2022

dlvenable Mar 8, 2022

chenqi0805 Mar 9, 2022

codecov-commenter commented Mar 9, 2022

sbayer55 Mar 9, 2022

sbayer55 Mar 9, 2022

ADD: ParseTreeCoercionService and tests #1153

ADD: ParseTreeCoercionService and tests #1153

Conversation

chenqi0805 commented Mar 8, 2022 • edited Loading

Description

Issues Resolved

Check List

sbayer55 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmanning09 Mar 8, 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

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

codecov-commenter commented Mar 9, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenqi0805 commented Mar 8, 2022 •

edited

Loading

cmanning09 Mar 8, 2022 •

edited

Loading