Skip to content

[refactor] Refactor the comparison logic #3574

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

Merged
MasamiYui merged 8 commits into from
Jul 19, 2025
Merged

[refactor] Refactor the comparison logic #3574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6122067
[refactor] Refactor the comparison logic
Duansg Jul 13, 2025
80c2509
Merge branch 'master' into refactor-comparison
Calvin979 Jul 13, 2025
b676782
Merge branch 'master' into refactor-comparison
Aias00 Jul 14, 2025
45bd783
Merge branch 'master' into refactor-comparison
Calvin979 Jul 17, 2025
f6f70a0
Merge branch 'master' into refactor-comparison
Duansg Jul 18, 2025
f078b92
Merge branch 'master' into refactor-comparison
Duansg Jul 18, 2025
a3388b1
Merge branch 'master' into refactor-comparison
Duansg Jul 18, 2025
4957182
Merge branch 'master' into refactor-comparison
tomsun28 Jul 19, 2025
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
205 changes: 157 additions & 48 deletions ...eat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionBaseVisitor.java
View file
Open in desktop

Large diffs are not rendered by default.

225 changes: 127 additions & 98 deletions ...eat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@
package org.apache.hertzbeat.alert.expr;

import org.antlr.v4.runtime.CommonTokenStream;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.hertzbeat.common.support.exception.ExpressionVisitorException;
import org.apache.hertzbeat.warehouse.db.QueryExecutor;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand All @@ -33,7 +34,7 @@
*/
public class AlertExpressionEvalVisitor extends AlertExpressionBaseVisitor<List<Map<String, Object>>> {

private static final String THRESHOLD = "__threshold__";
private static final String SCALAR = "__scalar__";
private static final String NAME = "__name__";
private static final String VALUE = "__value__";
private static final String TIMESTAMP = "__timestamp__";
Expand All @@ -60,28 +61,100 @@ public List<Map<String, Object>> visitParenExpr(AlertExpressionParser.ParenExprC
public List<Map<String, Object>> visitComparisonExpr(AlertExpressionParser.ComparisonExprContext ctx) {
List<Map<String, Object>> leftResult = visit(ctx.left);
List<Map<String, Object>> rightResult = visit(ctx.right);
if (rightResult.size() == 1 && rightResult.get(0).containsKey(THRESHOLD)) {
double threshold = (double) rightResult.get(0).get(THRESHOLD);
String operator = ctx.op.getText();

List<Map<String, Object>> result = new ArrayList<>();
for (Map<String, Object> item : leftResult) {
Object queryValues = item.get(VALUE);
if (queryValues == null) {
// ignore the query result data is empty
int type = ctx.op.getType();
boolean boolModifier = ctx.BOOL() != null;
boolean leftIsScalar = isScalar(leftResult);
boolean rightIsScalar = isScalar(rightResult);
List<Map<String, Object>> results = new ArrayList<>();

// scalar and scalar
if (leftIsScalar && rightIsScalar) {
if (!boolModifier) {
// Between two scalars,
// the bool modifier must be provided and these operators result in another scalar that is either 0 (false) or 1 (true), depending on the comparison result.
return results;
}
Object leftVal = leftResult.get(0).get(SCALAR);
Object rightVal = rightResult.get(0).get(SCALAR);
Boolean match = compareOp(leftVal, type, rightVal);
// returns a result only if the comparison condition is met.
Map<String, Object> result = new HashMap<>();
result.put(VALUE, match ? 1 : 0);
results.add(result);
return results;
}

// scalar and vector
if (leftIsScalar) {
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The branches for scalar–vector and vector–scalar comparisons share very similar logic. Consider extracting a helper method to reduce duplication.

Copilot uses AI. Check for mistakes.
Object leftVal = leftResult.get(0).get(SCALAR);
for (Map<String, Object> rightItem : rightResult) {
Object rightVal = rightItem.getOrDefault(VALUE, null);
if (isValidValue(rightVal)) {
continue;
}
// queryValues may be a list of values, or a single value
Object matchValue = evaluateCondition(queryValues, operator, threshold);
Map<String, Object> resultMap = new HashMap(item);
resultMap.put(VALUE, matchValue);
// if matchValue is null, mean not match the threshold
// if not null, mean match the threshold
result.add(resultMap);
Boolean match = compareOp(leftVal, type, rightVal);
Map<String, Object> result = new HashMap<>(rightItem);
if (boolModifier) {
result.put(VALUE, match ? 1 : 0);
results.add(result);
} else {
result.put(VALUE, match ? rightVal : null);
results.add(result);
}
}
return results;
}

// vector and scalar
if (rightIsScalar) {
Object rightVal = rightResult.get(0).get(SCALAR);
for (Map<String, Object> leftItem : leftResult) {
Object leftVal = leftItem.getOrDefault(VALUE, null);
if (isValidValue(leftVal)) {
continue;
}
Boolean match = compareOp(leftVal, type, rightVal);
Map<String, Object> result = new HashMap<>(leftItem);
if (boolModifier) {
result.put(VALUE, match ? 1 : 0);
results.add(result);
} else {
result.put(VALUE, match ? leftVal : null);
results.add(result);
}
}
return results;
}

// vector and vector
Map<String, Map<String, Object>> rightMap = rightResult.stream()
.filter(item -> item.get(VALUE) != null)
.collect(Collectors.toMap(this::labelKey, item -> item, (existing, replacement) -> existing));

for (Map<String, Object> leftItem : leftResult) {
Object leftVal = leftItem.getOrDefault(VALUE, null);
if (isValidValue(leftVal)) {
continue;
}
Map<String, Object> rightItem = rightMap.get(labelKey(leftItem));
if (rightItem == null) {
continue;
}
Object rightVal = rightItem.get(VALUE);
if (isValidValue(rightVal)) {
continue;
}
Boolean match = compareOp(leftVal, type, rightVal);
Map<String, Object> result = new HashMap<>(leftItem);
if (boolModifier) {
result.put(VALUE, match ? 1 : 0);
results.add(result);
} else {
result.put(VALUE, match ? leftVal : null);
results.add(result);
}
return result;
}
return new LinkedList<>();
return results;
}

@Override
Expand Down Expand Up @@ -167,7 +240,7 @@ public List<Map<String, Object>> visitLiteralExpr(AlertExpressionParser.LiteralE
double value = Double.parseDouble(ctx.number().getText());
List<Map<String, Object>> numAsList = new ArrayList<>();
Map<String, Object> valueMap = new HashMap<>();
valueMap.put(THRESHOLD, value);
valueMap.put(SCALAR, value);
numAsList.add(valueMap);
return numAsList;
}
Expand All @@ -194,83 +267,7 @@ public List<Map<String, Object>> visitPromqlCallExpr(AlertExpressionParser.Promq
return callSqlOrPromql(tokens.getText(ctx.string()));
}

private Object evaluateCondition(Object value, String operator, Double threshold) {
// value may be a list of values, or a single value
switch (operator) {
case ">":
// if value is list, return the max value
if (value instanceof List<?> values) {
Double doubleValue = values.stream().map(v -> Double.valueOf(v.toString())).max(Double::compareTo).orElse(null);
if (doubleValue != null) {
return doubleValue > threshold ? doubleValue : null;
} else {
return null;
}
} else {
return Double.parseDouble(value.toString()) > threshold ? value : null;
}
case ">=":
if (value instanceof List<?> values) {
Double doubleValue = values.stream().map(v -> Double.valueOf(v.toString())).max(Double::compareTo).orElse(null);
if (doubleValue != null) {
return doubleValue >= threshold ? doubleValue : null;
} else {
return null;
}
} else {
return Double.parseDouble(value.toString()) >= threshold ? value : null;
}
case "<":
if (value instanceof List<?> values) {
Double doubleValue = values.stream().map(v -> Double.valueOf(v.toString())).min(Double::compareTo).orElse(null);
if (doubleValue != null) {
return doubleValue < threshold ? doubleValue : null;
} else {
return null;
}
} else {
return Double.parseDouble(value.toString()) < threshold ? value : null;
}
case "<=":
if (value instanceof List<?> values) {
Double doubleValue = values.stream().map(v -> Double.valueOf(v.toString())).min(Double::compareTo).orElse(null);
if (doubleValue != null) {
return doubleValue <= threshold ? doubleValue : null;
} else {
return null;
}
} else {
return Double.parseDouble(value.toString()) <= threshold ? value : null;
}
case "==":
if (value instanceof List<?> values) {
for (Object v : values) {
if (v.equals(threshold)) {
return v;
}
}
return null;
} else {
return value.equals(threshold) ? value : null;
}
case "!=":
if (value instanceof List<?> values) {
for (Object v : values) {
if (v.equals(threshold)) {
return null;
}
}
return value;
} else {
return value.equals(threshold) ? null : value;
}
default:
// unsupported operator todo add more operator
return null;
}
}

private List<Map<String, Object>> callSqlOrPromql(String text){
private List<Map<String, Object>> callSqlOrPromql(String text) {
String script = text.substring(1, text.length() - 1);
return executor.execute(script);
}
Expand All @@ -290,4 +287,36 @@ private String labelKey(Map<String, Object> labelsMap) {
return key.isEmpty() ? "-" : key;
}

private boolean isScalar(List<Map<String, Object>> context) {
return CollectionUtils.isNotEmpty(context)
&& context.size() == 1
&& context.get(0).containsKey(SCALAR)
&& null != context.get(0).get(SCALAR);
}

private double parseStrToDouble(String text) {
try {
return Double.parseDouble(text);
} catch (NumberFormatException e) {
throw new ExpressionVisitorException("number format exception", e);
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

The error message "number format exception" is too generic. Include the input text that failed to parse for better troubleshooting.

Suggested change
throw new ExpressionVisitorException("number format exception", e);
throw new ExpressionVisitorException("Failed to parse '" + text + "' as a double", e);

Copilot uses AI. Check for mistakes.
}
}

private boolean isValidValue(Object val) {
return val == null || val instanceof List<?>;
}

private Boolean compareOp(Object leftVal, int opType, Object rightVal) {
double left = parseStrToDouble(leftVal.toString());
double right = parseStrToDouble(rightVal.toString());
return switch (opType) {
case AlertExpressionParser.GT -> left > right;
case AlertExpressionParser.GE -> left >= right;
case AlertExpressionParser.LT -> left < right;
case AlertExpressionParser.LE -> left <= right;
case AlertExpressionParser.EQ -> left == right;
Copy link

Copilot AI Jul 14, 2025

Choose a reason for hiding this comment

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

[nitpick] Using == to compare doubles can be unreliable due to floating‐point precision. Consider using Double.compare(left, right) == 0 or a tolerance-based comparison.

Suggested change
case AlertExpressionParser.EQ -> left == right;
case AlertExpressionParser.EQ -> Double.compare(left, right) == 0;

Copilot uses AI. Check for mistakes.
case AlertExpressionParser.NE -> left != right;
default -> false;
};
}
}
Loading
Loading