New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FEAT: parse tree evaulator and listener #1169

Merged

chenqi0805 merged 21 commits into opensearch-project:main from chenqi0805:feat/1003-parse-tree-evaulator-and-listener

Mar 10, 2022

Collaborator

chenqi0805 commented Mar 10, 2022

Description

This PR

adds ParseTreeEvaluator and ParseTreeEvaluatorListener
adjusts Operator to expose getNumberOfOperands
adjusts OperatorProvider to expose containsOperatorSymbol

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.

chenqi0805 added 16 commits

March 7, 2022 20:26


          ADD: ParseTreeCoercionService and tests

62b2f21

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


          ENH: add rule index in operators

4ab6d99

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


          ADD: OperatorProvider to encapsulate operator retrieval

3635d1e

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


          MNT: reuse nodeStringValue

f3edf68

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


          MAINT: address all PR comments

74a6799

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]>


          MAINT: use more generic interface

371e31e

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]>


          MNT: separate variable

40712f0

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


          Merge branch 'enh/1003-operators-and-providers' into feat/1003-parse-…

d0db7b3

…tree-evaulator-and-listener

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


          ADD: ParseTreeEvaluator and ParseTreeEvaluatorListener

f54754c

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


          MAINT: javadoc

8861fc2

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


          TST: visit error node

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


          MAINT: merge and resolve conflict with current main

ee21aa6

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

chenqi0805 requested a review from a team as a code owner

March 10, 2022 15:18

chenqi0805 added 3 commits

March 10, 2022 09:35


          FIX: more conflicts

dc4dcd0

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


          STY: unused imports

cd7c0be

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


          ENH: expression coercion exception runtime

e66d10d

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

chenqi0805 requested review from sbayer55, dlvenable and cmanning09

March 10, 2022 16:05

chenqi0805 mentioned this pull request

Negative value support with unary subtract operator #1165

Merged

4 tasks

chenqi0805 changed the title ~~FEAT: parse tree evaulator and listener~~ [Blocked] FEAT: parse tree evaulator and listener

Collaborator Author

chenqi0805 commented Mar 10, 2022

This PR needs to receive modification after merging of #1165

dlvenable requested changes

View reviewed changes

...prepper-expression/src/main/java/org/opensearch/dataprepper/expression/OperatorProvider.java Outdated

@@ @@ -35,4 +35,8 @@ public OperatorProvider(final List<Operator<?>> operators) { @@
                       }
                       return operator;
                   }
+                  public boolean containsOperatorSymbol(final int symbol) {

Member

dlvenable Mar 10, 2022

As we already have getOperator, should this be have a different name? Perhaps: containsOperator or supportsOperator?

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

+               * walker.walk(listener, ...);
+               * final Object result = listener.getResult();
+               */
+              class ParseTreeEvaluatorListener extends DataPrepperExpressionBaseListener {

Member

dlvenable Mar 10, 2022

Nice! I like how this came out.

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

+               * walker.walk(listener, ...);
+               * final Object result = listener.getResult();
+               */
+              class ParseTreeEvaluatorListener extends DataPrepperExpressionBaseListener {

Member

dlvenable Mar 10, 2022

Nice! I like how this came out.

...r-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorTest.java Outdated

+                  void testEvaluateSuccess() throws ExpressionCoercionException {
+                      try (final MockedConstruction<ParseTreeEvaluatorListener> ignored =
+                                   mockConstruction(ParseTreeEvaluatorListener.class, (mock, context) -> when(mock.getResult()).thenReturn(true))) {
+                          when(coercionService.coerce(any(), any())).thenAnswer(invocation -> {

Member

dlvenable Mar 10, 2022

I don't believe you need this when in the try-with-resources. Moving it out will be clearer.

...r-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorTest.java Outdated

+                  void testEvaluateSuccess() throws ExpressionCoercionException {
+                      try (final MockedConstruction<ParseTreeEvaluatorListener> ignored =
+                                   mockConstruction(ParseTreeEvaluatorListener.class, (mock, context) -> when(mock.getResult()).thenReturn(true))) {
+                          when(coercionService.coerce(any(), any())).thenAnswer(invocation -> {

Member

dlvenable Mar 10, 2022

I don't believe you need this when in the try-finally.

...r-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorTest.java Outdated

+                  void testEvaluateFailureInWalk() {
+                      try (final MockedConstruction<ParseTreeEvaluatorListener> ignored =
+                                   mockConstruction(ParseTreeEvaluatorListener.class)) {
+                          doThrow(new RuntimeException()).when(parseTreeWalker).walk(

Member

dlvenable Mar 10, 2022

Please move doThrow out of try-with-resources.

...r-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorTest.java Outdated

+                  void testEvaluateFailureInCoerce() throws ExpressionCoercionException {
+                      try (final MockedConstruction<ParseTreeEvaluatorListener> ignored =
+                                   mockConstruction(ParseTreeEvaluatorListener.class, (mock, context) -> when(mock.getResult()).thenReturn(true))) {
+                          when(coercionService.coerce(any(), any())).thenThrow(new ExpressionCoercionException("test message"));

Member

dlvenable Mar 10, 2022

Please move when out of try-with-resources.

...r-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorTest.java Outdated

+                  void testEvaluateFailureInCoerce() throws ExpressionCoercionException {
+                      try (final MockedConstruction<ParseTreeEvaluatorListener> ignored =
+                                   mockConstruction(ParseTreeEvaluatorListener.class, (mock, context) -> when(mock.getResult()).thenReturn(true))) {
+                          when(coercionService.coerce(any(), any())).thenThrow(new ExpressionCoercionException("test message"));

Member

dlvenable Mar 10, 2022

Please move when out of try-with-resources.

...r-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorTest.java Outdated

+                          when(coercionService.coerce(any(), any())).thenAnswer(invocation -> {
+                              Object[] args = invocation.getArguments();
+                              final Object res = args[0];
+                              final Class<Boolean> clazz = (Class<Boolean>) args[1];

Member

dlvenable Mar 10, 2022

I think that invocation has a method that will cast for you. getArgument(1) or something like that.

...r-expression/src/test/java/org/opensearch/dataprepper/expression/ParseTreeEvaluatorTest.java Outdated

+                          when(coercionService.coerce(any(), any())).thenAnswer(invocation -> {
+                              Object[] args = invocation.getArguments();
+                              final Object res = args[0];
+                              final Class<Boolean> clazz = (Class<Boolean>) args[1];

Member

dlvenable Mar 10, 2022

I think that invocation has a method that will cast for you. getArgument(1) or something like that.


          MAINT: address PR comments

ce1121d

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

dlvenable previously approved these changes

View reviewed changes

...prepper-expression/src/main/java/org/opensearch/dataprepper/expression/OperatorProvider.java Outdated

@@ @@ -35,4 +35,8 @@ public OperatorProvider(final List<Operator<?>> operators) { @@
                       }
                       return operator;
                   }
+                  public boolean containsOperatorSymbol(final int symbol) {

Member

dlvenable Mar 10, 2022

As we already have getOperator, should this be have a different name? Perhaps: containsOperator or supportsOperator?


          MAINT: merge current main and resolve conflicts

5779a86

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

chenqi0805 dismissed dlvenable’s stale review via

5779a86

March 10, 2022 20:49

chenqi0805 changed the title ~~[Blocked] FEAT: parse tree evaulator and listener~~ FEAT: parse tree evaulator and listener

chenqi0805 requested a review from dlvenable

March 10, 2022 20:49

chenqi0805 mentioned this pull request

New operators #1172

Merged

4 tasks

dlvenable approved these changes

View reviewed changes

sbayer55 reviewed

View reviewed changes

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

+                          throw new IllegalStateException("The ParseTreeEvaluatorListener has not been walked through exactly once by " +
+                                  "a ParseTreeWalker.");
+                      }
+                      return operandStack.peek();

Member

sbayer55 Mar 10, 2022

It might be safer to pop to prevent reuse of a listener considering each evaluate call needs a listener.

Collaborator Author

chenqi0805 Mar 10, 2022 •

edited

Loading

The reason I did not use pop is based off idempotency. It might be a little weird that the second getResult() call throws exception when called in succession.

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

+                          operatorSymbolStack.push(nodeType);
+                      } else if (nodeType == DataPrepperExpressionParser.RPAREN) {
+                          // pop LPAREN at operatorSymbolStack top
+                          operatorSymbolStack.pop();

Member

sbayer55 Mar 10, 2022

Consider verifying the top of the stack is a LPAREN.

Collaborator Author

chenqi0805 Mar 10, 2022

Do you mean throw exception if it is not LPAREN? AFAICT, it should only happen if parentheses in the expression are unbalanced, which should already be ruled out by parser?

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

+                  private void performSingleOperation(final Operator<?> operator) {
+                      final int numOfArgs = operator.getNumberOfOperands();
+                      final Object[] args = new Object[numOfArgs];
+                      for (int i = numOfArgs - 1; i >= 0; i--) {

Member

sbayer55 Mar 10, 2022

Checking operandStack.size() >= numOfArgs and throwing a more informative error message may lead to a better customer experience.

Collaborator Author

chenqi0805 Mar 10, 2022 •

edited

Loading

Not sure if it is necessary. Here is the reasoning: I suppose if the input parseTree is valid, we should never hit this error unless the evaluation algorithm is incorrect and fails to apply to certain corner cases. I do not think we should use exception or error message to guard against incorrect implementation. Also, the error message thrown here is not that helpful to cx on troubleshooting their if condition statement.

sbayer55 approved these changes

View reviewed changes

chenqi0805 merged commit 6394263 into opensearch-project:main

chenqi0805 deleted the feat/1003-parse-tree-evaulator-and-listener branch

March 10, 2022 21:45

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet