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

FEAT: operators in data-prepper expression #1065

Merged
merged 15 commits into from
Mar 3, 2022
Merged

FEAT: operators in data-prepper expression #1065

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fce1e77
FEAT: operators and tests
chenqi0805 Feb 18, 2022
0106e92
MAINT: lower -> less
chenqi0805 Feb 24, 2022
cc492ea
MAINT: use int for symbol
chenqi0805 Feb 24, 2022
d175a29
MAINT: include null in equality operators
chenqi0805 Feb 24, 2022
bb9669b
MAINT: operator classes package private
chenqi0805 Feb 28, 2022
3c7d917
STY: eval -> evaluate
chenqi0805 Feb 28, 2022
6fe9dfc
MAINT: simplify numerical value comparison operators
chenqi0805 Feb 28, 2022
da97f79
MAINT: DI annotation on operators
chenqi0805 Mar 1, 2022
9e66532
MAINT: OperatorFactory to produce negate and numeric comparison operator
chenqi0805 Mar 1, 2022
b57a1df
Merge branch 'main' into feat/1003-add-operator-for-data-prepper-expr…
chenqi0805 Mar 1, 2022
50f47df
MAINT: use NumericCompareOperator
chenqi0805 Mar 2, 2022
73a2d71
MAINT: generic operator to cover negate
chenqi0805 Mar 2, 2022
5276cef
MAINT: generic operators
chenqi0805 Mar 2, 2022
90817cf
STY: static final
chenqi0805 Mar 2, 2022
da229ec
MNT: Objects::equals
chenqi0805 Mar 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/AndOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import static com.google.common.base.Preconditions.checkArgument;

class AndOperator implements Operator<Boolean> {
@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.AND;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
checkArgument(args[0] instanceof Boolean, "Left operand needs to be Boolean.");
checkArgument(args[1] instanceof Boolean, "Right Operand needs to be Boolean.");
return ((Boolean) args[0]) && ((Boolean) args[1]);
}
}
12 changes: 12 additions & 0 deletions ...ression/src/main/java/org/opensearch/dataprepper/expression/BinaryOperandsKeyFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

class BinaryOperandsKeyFactory {
static String typesKey(final Object leftValue, final Object rightValue) {
return leftValue.getClass().getName() + "_" + rightValue.getClass().getName();
}
}
27 changes: 27 additions & 0 deletions ...prepper-expression/src/main/java/org/opensearch/dataprepper/expression/EqualOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import static com.google.common.base.Preconditions.checkArgument;

class EqualOperator implements Operator<Boolean> {

@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.EQUAL;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
if ((args[0] == null) || (args[1] == null)) {
return args[0] == null && args[1] == null;
}
return args[0].equals(args[1]);
}
}
41 changes: 41 additions & 0 deletions ...r-expression/src/main/java/org/opensearch/dataprepper/expression/GreaterThanOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import java.util.HashMap;
import java.util.Map;
import java.util.function.BiFunction;

import static com.google.common.base.Preconditions.checkArgument;

class GreaterThanOperator implements Operator<Boolean> {
private static final Map<String, BiFunction<Object, Object, Boolean>> GREATER_THAN_ON =
new HashMap<String, BiFunction<Object, Object, Boolean>>() {{
put(Integer.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Integer) a > (Integer) b);
put(Integer.class.getName() + "_" + Float.class.getName(), (a, b) -> (Integer) a > (Float) b);
put(Float.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Float) a > (Integer) b);
put(Float.class.getName() + "_" + Float.class.getName(), (a, b) -> (Float) a > (Float) b);}};

@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.GT;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a higher level question for @sbayer55. What the user experience like with this error message actionable for our users? How will I, as a user, know which Operands length needs to be adjusted?

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 do not have the full context. But my guess is that L31 should always given the expression parser and correct parseTreeEvaluator evaluation logic that is supposed to be consistent with the ANTLR grammar

Copy link
Member

Choose a reason for hiding this comment

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

There are a couple of safety checks before the operator is called.

  1. The Grammar should enforce that operands are always provided the correct number of arguments
  2. The Operator provides a required number of arguments that will be pulled from the arg stack before evaluate is called.

The exception message could be more specific, saying something like "Greater than operator requires two arguments". The evaluator class will then catch that exception and can provide more context on where in the statement the error occurred. I don't think it would make sense to expose that level of information to the operator instance.

Issue 1003 has a sequence flow diagram to give additional context.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sbayer55 Thanks. IMO, to provide more specific exception message with operator type encoded, we might need to rethink on the design of NumericCompareOperator or NegateOperator that have been proposed. @cmanning09 thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After a second look, we may inject operator type string e.g. ">=" into NumericCompareOperator but have not yet found a way for NegateOperator.

Copy link
Member

Choose a reason for hiding this comment

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

Have you considered a negate operator that internally holds a reference to another operator whose results are negated?

final Object leftValue = args[0];
final Object rightValue = args[1];
final BiFunction<Object, Object, Boolean> operation = GREATER_THAN_ON.get(
BinaryOperandsKeyFactory.typesKey(leftValue, rightValue));
if (operation == null) {
throw new IllegalArgumentException(leftValue + " or " + rightValue + " should be either Float or Integer");
}
return operation.apply(leftValue, rightValue);
}
}
41 changes: 41 additions & 0 deletions ...ssion/src/main/java/org/opensearch/dataprepper/expression/GreaterThanOrEqualOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import java.util.HashMap;
import java.util.Map;
import java.util.function.BiFunction;

import static com.google.common.base.Preconditions.checkArgument;

class GreaterThanOrEqualOperator implements Operator<Boolean> {
private static final Map<String, BiFunction<Object, Object, Boolean>> GREATER_THAN_OR_EQUAL_ON =
new HashMap<String, BiFunction<Object, Object, Boolean>>() {{
put(Integer.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Integer) a >= (Integer) b);
put(Integer.class.getName() + "_" + Float.class.getName(), (a, b) -> (Integer) a >= (Float) b);
put(Float.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Float) a >= (Integer) b);
put(Float.class.getName() + "_" + Float.class.getName(), (a, b) -> (Float) a >= (Float) b);}};

@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.GTE;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
final Object leftValue = args[0];
final Object rightValue = args[1];
final BiFunction<Object, Object, Boolean> operation = GREATER_THAN_OR_EQUAL_ON.get(
BinaryOperandsKeyFactory.typesKey(leftValue, rightValue));
if (operation == null) {
throw new IllegalArgumentException(leftValue + " or " + rightValue + " should be either Float or Integer");
}
return operation.apply(leftValue, rightValue);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like @dinujoh 's suggestion and I think we can continue in this direction to reduce the duplicate code as well. Now that the operation logic has been reduced to a map, we can pull out all the boilerplate code and inject the unique map for each operator through a constructor. The typesKey method doesn't have to be a static utility function either and can reside in the class.

class NumericComparisonOperator implements Operator<Boolean> {

    private final Integer symbol;
    private final Map<String, BiFunction<Object, Object, Boolean>> operationMap;

    public GenericComparisonOperator(final Integer symbol, final Map<String, BiFunction<Object, Object, Boolean>> operationMap) {
        this.symbol = symbol;
        this.operationMap = operationMap;
    }

    public getSymbol() {
        return this.symbol;
    }

        @Override
    public Boolean evaluate(final Object... args) {
        checkArgument(args.length == 2, "Operands length needs to be 2.");
        final Object leftValue = args[0];
        final Object rightValue = args[1];
        final BiFunction<Object, Object, Boolean> operation = operationMap.get(
                BinaryOperandsKeyFactory.typesKey(leftValue, rightValue));
        if (operation == null) {
            throw new IllegalArgumentException(leftValue + " or " + rightValue + " should be either Float or Integer");
        }
        return operation.apply(leftValue, rightValue);
    }

    private String typesKey(final Object leftValue, final Object rightValue) {
        return leftValue.getClass().getName() + "_" + rightValue.getClass().getName();
    }
}

Then we can build these instances as part of our DI.

    private static final Map<String, BiFunction<Object, Object, Boolean>> GREATER_THAN_OR_EQUAL_ON =
            new HashMap<String, BiFunction<Object, Object, Boolean>>() {{
                put(Integer.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Integer) a >= (Integer) b);
                put(Integer.class.getName() + "_" + Float.class.getName(), (a, b) -> (Integer) a >= (Float) b);
                put(Float.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Float) a >= (Integer) b);
                put(Float.class.getName() + "_" + Float.class.getName(), (a, b) -> (Float) a >= (Float) b);}};

   public Operator<Boolean> greaterThanOrEqualToBuilder() {
        return new NumericComparisonOperator(DataPrepperExpressionParser.GTE, GREATER_THAN_OR_EQUAL_ON);
    }

    class Operator<Boolean> lessThanOrEqualToBuilder() {
         // LESS_THAN map...
    }

    // etc.
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIUC, we group all NumericComparisonOperator instantiation into a factory class?

}
28 changes: 28 additions & 0 deletions data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/InOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import java.util.Set;

import static com.google.common.base.Preconditions.checkArgument;

class InOperator implements Operator<Boolean> {
@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.IN_SET;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
if (!(args[1] instanceof Set)) {
throw new IllegalArgumentException(args[1] + " should be Set");
}
return ((Set<?>) args[1]).contains(args[0]);
}
}
41 changes: 41 additions & 0 deletions ...pper-expression/src/main/java/org/opensearch/dataprepper/expression/LessThanOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import java.util.HashMap;
import java.util.Map;
import java.util.function.BiFunction;

import static com.google.common.base.Preconditions.checkArgument;

class LessThanOperator implements Operator<Boolean> {
private static final Map<String, BiFunction<Object, Object, Boolean>> LESS_THAN_ON =
new HashMap<String, BiFunction<Object, Object, Boolean>>() {{
put(Integer.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Integer) a < (Integer) b);
put(Integer.class.getName() + "_" + Float.class.getName(), (a, b) -> (Integer) a < (Float) b);
put(Float.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Float) a < (Integer) b);
put(Float.class.getName() + "_" + Float.class.getName(), (a, b) -> (Float) a < (Float) b);}};

@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.LT;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
final Object leftValue = args[0];
final Object rightValue = args[1];
final BiFunction<Object, Object, Boolean> operation = LESS_THAN_ON.get(
BinaryOperandsKeyFactory.typesKey(leftValue, rightValue));
if (operation == null) {
throw new IllegalArgumentException(leftValue + " or " + rightValue + " should be either Float or Integer");
}
return operation.apply(leftValue, rightValue);
}
}
41 changes: 41 additions & 0 deletions ...pression/src/main/java/org/opensearch/dataprepper/expression/LessThanOrEqualOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import java.util.HashMap;
import java.util.Map;
import java.util.function.BiFunction;

import static com.google.common.base.Preconditions.checkArgument;

class LessThanOrEqualOperator implements Operator<Boolean> {
private static final Map<String, BiFunction<Object, Object, Boolean>> LESS_THAN_OR_EQUAL_ON =
new HashMap<String, BiFunction<Object, Object, Boolean>>() {{
put(Integer.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Integer) a <= (Integer) b);
put(Integer.class.getName() + "_" + Float.class.getName(), (a, b) -> (Integer) a <= (Float) b);
put(Float.class.getName() + "_" + Integer.class.getName(), (a, b) -> (Float) a <= (Integer) b);
put(Float.class.getName() + "_" + Float.class.getName(), (a, b) -> (Float) a <= (Float) b);}};

@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.LTE;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
final Object leftValue = args[0];
final Object rightValue = args[1];
final BiFunction<Object, Object, Boolean> operation = LESS_THAN_OR_EQUAL_ON.get(
BinaryOperandsKeyFactory.typesKey(leftValue, rightValue));
if (operation == null) {
throw new IllegalArgumentException(leftValue + " or " + rightValue + " should be either Float or Integer");
}
return operation.apply(leftValue, rightValue);
}
}
27 changes: 27 additions & 0 deletions ...pper-expression/src/main/java/org/opensearch/dataprepper/expression/NotEqualOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import static com.google.common.base.Preconditions.checkArgument;

class NotEqualOperator implements Operator<Boolean> {

@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.NOT_EQUAL;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
if ((args[0] == null) || (args[1] == null)) {
return args[0] != null || args[1] != null;
}
return !args[0].equals(args[1]);
}
}
27 changes: 27 additions & 0 deletions ...prepper-expression/src/main/java/org/opensearch/dataprepper/expression/NotInOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import java.util.Set;

import static com.google.common.base.Preconditions.checkArgument;

class NotInOperator implements Operator<Boolean> {

@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.NOT_IN_SET;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
checkArgument(args[1] instanceof Set, "Right Operand needs to be Set.");
return !((Set<?>) args[1]).contains(args[0]);
}
}
24 changes: 24 additions & 0 deletions data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/NotOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import static com.google.common.base.Preconditions.checkArgument;

class NotOperator implements Operator<Boolean> {
@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.NOT;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 1, "Operands length needs to be 1.");
checkArgument(args[0] instanceof Boolean, "Operand needs to be Boolean.");
return !((Boolean) args[0]);
}
}
19 changes: 19 additions & 0 deletions data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/Operator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

interface Operator<T> {
Integer getSymbol();
Comment on lines +8 to +9
Copy link
Contributor

Choose a reason for hiding this comment

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

All interfaces should be documented. Please add java docs.


/**
* @since 1.3
* Placeholder interface for implementing Data-Prepper supported binary/unary operations on operands that
* returns custom type T.
* @param args operands
* @return T
*/
T evaluate(final Object... args);
}
26 changes: 26 additions & 0 deletions data-prepper-expression/src/main/java/org/opensearch/dataprepper/expression/OrOperator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

package org.opensearch.dataprepper.expression;

import org.opensearch.dataprepper.expression.antlr.DataPrepperExpressionParser;

import static com.google.common.base.Preconditions.checkArgument;

class OrOperator implements Operator<Boolean> {

@Override
public Integer getSymbol() {
return DataPrepperExpressionParser.OR;
}

@Override
public Boolean evaluate(Object... args) {
checkArgument(args.length == 2, "Operands length needs to be 2.");
checkArgument(args[0] instanceof Boolean, "Left operand needs to be Boolean.");
checkArgument(args[1] instanceof Boolean, "Right Operand needs to be Boolean.");
return ((Boolean) args[0]) || ((Boolean) args[1]);
}
}
Loading