[refactor] Refactor the comparison logic

[Duansg]

What's changed?

Refactor the comparison logic in org.apache.hertzbeat.alert.expr.AlertExpressionEvalVisitor#visitComparisonExpr

For details:

1. Optimized the logic of historical comparison binary operators.
2. Improve the semantics of promql, add the comparison binary operators function, and add scalar and scalar, scalar and vector, and vector and vector.
3. Antlr4 added the bool modifier.

4. Compatible with current alarm logic processing.
5. Complete/fix test cases and add new test cases.

Checklist

• I have read the Contributing Guide
• I have written the necessary doc or comment.
• I have added the necessary unit tests and all cases have passed.

Add or update API

• I have added the necessary e2e tests and all cases have passed.

// vector and vector

// vector and scalar

// scalar and vector

// scalar and scalar

[Copilot]

Pull Request Overview

This PR refactors the comparison logic in AlertExpressionEvalVisitor to fully support PromQL-style comparisons with an optional bool modifier and combinations of scalar/vector operands.

• Added optional bool modifier and distinct handling for scalar–scalar, scalar–vector, vector–scalar, and vector–vector comparisons.
• Updated the ANTLR grammar (AlertExpression.g4) to allow an optional BOOL token in comparison expressions.
• Added new unit tests in AlertExpressionEvalVisitorTest covering all comparison scenarios.

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.

File	Description
hertzbeat-common/src/main/java/org/apache/hertzbeat/common/support/exception/ExpressionVisitorException.java	Introduces a custom ExpressionVisitorException for parse errors
hertzbeat-alerter/src/main/resources/expr/AlertExpression.g4	Extends the comparison rule to include an optional BOOL modifier
hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java	Refactors visitComparisonExpr to handle scalar/vector and bool
hertzbeat-alerter/src/test/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitorTest.java	Updates existing tests and adds new ones for comparison expressions

Comments suppressed due to low confidence (2)

hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java:305

• [nitpick] Method name isValidValue is misleading because it returns true for invalid values (null or List). Consider renaming to isInvalidValue or inverting the logic for clarity.

    private boolean isValidValue(Object val) {

hertzbeat-alerter/src/main/java/org/apache/hertzbeat/alert/expr/AlertExpressionEvalVisitor.java:132

• Missing import for java.util.stream.Collectors, which will cause a compile error when calling collect(Collectors.toMap(...)).

                .collect(Collectors.toMap(this::labelKey, item -> item, (existing, replacement) -> existing));

[Copilot]

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

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]

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

[MasamiYui]

Thank you for your PR.

There is an example I would like to discuss with you. According to the current parsing rules, http_requests_time > avg(rate(http_requests_time[5m])) will be parsed as vector > vector, but in reality, it should be vector > scalar.

At present, only LiteralExpr(number) is regarded as a scalar. I think more judgment conditions may be needed to determine whether something is a scalar, such as whether there are labels in the metric query results.

[Duansg]

avg(rate(http_requests_time[5m]))

Thank you for your PR.

There is an example I would like to discuss with you. According to the current parsing rules, http_requests_time > avg(rate(http_requests_time[5m])) will be parsed as vector > vector, but in reality, it should be vector > scalar.

At present, only LiteralExpr(number) is regarded as a scalar. I think more judgment conditions may be needed to determine whether something is a scalar, such as whether there are labels in the metric query results.

Hi, @MasamiYui thank you for your review. There are a few points regarding your question.

1. As long as your query is of type "instant vector" (even if there is only one value), the result will be a vector. You can verify this in the Prometheus UI and HTTP API.
2. Aggregate functions such as avg, which you mentioned in your example, are not scalars in the true sense of the word. I consider them to be unlabeled scalars, and I believe that the expression you provided will not return a result.
3. The normal approach would be to use the scalar function to return the sample value as a scalar, but the current PromqlQueryExecutor does not have the ability to parse resultType. I am currently working on refactoring this.

[MasamiYui]

avg(rate(http_requests_time[5m]))

Thank you for your PR.
There is an example I would like to discuss with you. According to the current parsing rules, http_requests_time > avg(rate(http_requests_time[5m])) will be parsed as vector > vector, but in reality, it should be vector > scalar.
At present, only LiteralExpr(number) is regarded as a scalar. I think more judgment conditions may be needed to determine whether something is a scalar, such as whether there are labels in the metric query results.

Hi, @MasamiYui thank you for your review. There are a few points regarding your question.

1. As long as your query is of type "instant vector" (even if there is only one value), the result will be a vector. You can verify this in the Prometheus UI and HTTP API.
2. Aggregate functions such as avg, which you mentioned in your example, are not scalars in the true sense of the word. I consider them to be unlabeled scalars, and I believe that the expression you provided will not return a result.
3. The normal approach would be to use the scalar function to return the sample value as a scalar, but the current PromqlQueryExecutor does not have the ability to parse resultType. I am currently working on refactoring this.

👍 I got it!

[Duansg]

@MasamiYui Oh, right, as of this PR, the semantic correctness should all be handled. Next, we should discuss the plans we mentioned earlier. When is the next meeting? We can discuss that.

I remember @tomsun28 saying that the meeting time was posted on the public channel. How can I subscribe to this information?

[MasamiYui]

@MasamiYui Oh, right, as of this PR, the semantic correctness should all be handled. Next, we should discuss the plans we mentioned earlier. When is the next meeting? We can discuss that.

I remember @tomsun28 saying that the meeting time was posted on the public channel. How can I subscribe to this information?

@Duansg please ref to : mailing_lists

[MasamiYui]

LGTM.