Skip to content
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

Data Prepper expressions - Set operator fix #4818

Merged
merged 4 commits into from
Aug 14, 2024
Merged

Data Prepper expressions - Set operator fix #4818

merged 4 commits into from
Aug 14, 2024

Conversation

kkondaka
Copy link
Collaborator

@kkondaka kkondaka commented Aug 9, 2024

Description

Set operator 'in' and 'not in' are not working as expected. This change fixes this.

Issues Resolved

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • [ X] New functionality includes testing.
  • New functionality has a documentation issue. Please link to it in this PR.
    • 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.

@dlvenable
Copy link
Member

@kkondaka , This should resolve #3854 correct?

dlvenable
dlvenable requested changes Aug 12, 2024
View reviewed changes
...nsearch/src/main/java/org/opensearch/dataprepper/plugins/sink/opensearch/OpenSearchSink.java Outdated
if (StringUtils.equals(action, OpenSearchBulkActions.UPDATE.toString()) ||
StringUtils.equals(action, OpenSearchBulkActions.UPSERT.toString()) ||
StringUtils.equals(action, OpenSearchBulkActions.DELETE.toString())) {
if (StringUtils.equals(eventAction, OpenSearchBulkActions.UPDATE.toString()) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This appears unrelated and appears to be from another PR. Please remove this change.

...est/java/org/opensearch/dataprepper/expression/GenericExpressionEvaluator_ConditionalIT.java Outdated
@@ -214,6 +214,19 @@ private static Stream<Arguments> validExpressionArguments() {
Arguments.of("/sourceIp != null", event("{\"sourceIp\": [10, 20]}"), true),
Arguments.of("/sourceIp == null", event("{\"sourceIp\": [\"test\", \"test_two\"]}"), false),
Arguments.of("/sourceIp == null", event("{\"sourceIp\": {\"test\": \"test_two\"}}"), false),
Arguments.of("/value in {200.222, 300.333, 400}", event("{\"value\": 400.0}"), true),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use arguments. See https://github.com/opensearch-project/data-prepper/pull/4798/files#diff-720b03bb55dc916760c0e8106c53d4fd6422c00bc31feb0cb9a90f24ea66fb4e

...expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java Outdated
@@ -76,6 +79,55 @@ public Object coercePrimaryTerminalNode(final TerminalNode node, final Event eve
return longValue;
}
return Integer.valueOf(nodeStringValue);
case DataPrepperExpressionParser.SetInitializer:
String[] setMembers = nodeStringValue.trim().substring(1,nodeStringValue.length()-1).split(",");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case statement is too complicated already. Let's move this out.

...expression/src/main/java/org/opensearch/dataprepper/expression/ParseTreeCoercionService.java Outdated
if (stringMemberCount > 0 && stringMemberCount != setMembers.length) {
throw new RuntimeException("All set members should be of same type");
}
if (stringMemberCount == setMembers.length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting the types is something we do elsewhere for other literals right? We should remove this code in favor of using the code that supports existing literals.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code doesn't get called for each of the set members. Let me know if I am missing something.

data-prepper-expression/src/main/antlr/DataPrepperExpression.g4 Outdated
@@ -181,6 +186,13 @@ literal
| Null
;

fragment
SetLiteral
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want another fragment here. We should use the other literal and remove this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt doing something like /boolean_value in {true, false} doesn't make sense. Similarly, I felt checking for null in a set is not that useful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's not very useful, but the literals may grow as well. I think if we just have a literal, we can have a nice recursive structure. This is also similar to other programming languages.

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

fragment
SetMembers
: SetLiteral (SPACE* COMMA SPACE* SetLiteral)*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: SetLiteral (SPACE* COMMA SPACE* SetLiteral)*
: literal (SPACE* COMMA SPACE* literal)*

Let's use the same literal. Also, can we use primary as it used to be used? Perhaps sets of sets would be appropriate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe in future? I do not want to complicate this any further for now. We have documented this since the beginning without supporting it.

@kkondaka kkondaka force-pushed the set-operator-fix branch 4 times, most recently from 3b3603a to 8139544 Compare August 14, 2024 06:29
kkondaka and others added 3 commits August 14, 2024 06:30
@kkondaka
dplive1.yaml
afcd8da 
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
@kkondaka
Delete .github/workflows/static.yml
4661346 
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
Addressed review comments
62eff52 
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
Fixed check style errors
fd9ae06 
Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
dlvenable
dlvenable approved these changes Aug 14, 2024
View reviewed changes
Copy link
Member

@dlvenable dlvenable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this looks great!

@kkondaka kkondaka merged commit 91b6666 into opensearch-project:main Aug 14, 2024
45 of 47 checks passed
@dlvenable dlvenable mentioned this pull request Aug 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graytaylor0 graytaylor0 graytaylor0 approved these changes

@dlvenable dlvenable dlvenable approved these changes

@chenqi0805 chenqi0805 Awaiting requested review from chenqi0805 chenqi0805 is a code owner

@engechas engechas Awaiting requested review from engechas engechas is a code owner

@dinujoh dinujoh Awaiting requested review from dinujoh dinujoh is a code owner

@KarstenSchnitter KarstenSchnitter Awaiting requested review from KarstenSchnitter KarstenSchnitter is a code owner

@oeyh oeyh Awaiting requested review from oeyh oeyh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kkondaka @dlvenable @graytaylor0