Copilot commented on code in PR #13858:
URL: https://github.com/apache/skywalking/pull/13858#discussion_r3178116799
##########
docs/en/changes/changes.md:
##########
@@ -98,6 +98,7 @@
* Fix: `envoy-ai-gateway` metrics rules, make the metrics value return `0`
when the divisor is `0`.
* Fix: LAL compiler treated `(tag("x") as Integer) + (tag("y") as Integer)` as
string concatenation instead of numeric addition. Expressions like
`input_tokens + output_tokens < 10000` produced the concatenated string
`"2589115"` rather than the integer sum `2704`, so token-threshold conditions
never triggered `abort {}`. The compiler now detects all-numeric operands (cast
to `Integer` or `Long`) and emits proper `long` arithmetic.
Review Comment:
This new changelog entry now conflicts with the previous LAL bullet just
above it, which still says the fix emits `long` arithmetic for all-integer
operands. Leaving both bullets in the same release section makes it unclear
which behavior users should expect in this version.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -214,24 +388,122 @@ static void generateConditionValue(final StringBuilder
sb,
}
}
+ /**
+ * Render a numeric RHS for a comparison operator. The literal is emitted
+ * in a form compatible with the LHS resolved type so the comparison stays
+ * in the LHS's primitive space — no defensive widening to long.
+ *
+ * <p>Examples (LHS in parens):
+ * <ul>
+ * <li>(int) {@code < 10000} → {@code 10000}</li>
+ * <li>(long) {@code < 10000} → {@code 10000L}</li>
+ * <li>(double) {@code < 1.5} → {@code 1.5d}</li>
+ * <li>(int) {@code < 1.5} → {@code 1.5d} — JLS will widen LHS to
double</li>
+ * </ul>
+ */
static void generateConditionValueNumeric(
final StringBuilder sb,
final LALScriptModel.ConditionValue cv,
+ final Class<?> lhsType,
final LALClassGenerator.GenCtx genCtx) {
if (cv instanceof LALScriptModel.NumberConditionValue) {
- sb.append((long) ((LALScriptModel.NumberConditionValue) cv)
- .getValue()).append("L");
+ final LALScriptModel.NumberConditionValue ncv =
+ (LALScriptModel.NumberConditionValue) cv;
+ final String literal = ncv.getLiteral();
+ if (literal != null) {
+ sb.append(formatNumericLiteralForCompare(literal, lhsType));
+ } else {
+ // Synthesised value (no source text).
+ sb.append(formatDoubleAsLiteral(ncv.getValue(), lhsType));
+ }
} else if (cv instanceof LALScriptModel.ValueAccessConditionValue) {
- sb.append("h.toLong(");
- generateValueAccessObj(sb,
- ((LALScriptModel.ValueAccessConditionValue) cv).getValue(),
- null, genCtx);
- sb.append(")");
+ // RHS value access — emit the LHS-typed primitive form. If
+ // codegen managed to land it as a primitive (typed proto field,
+ // arithmetic), reuse lastRawChain widened to lhsType; otherwise
+ // wrap the boxed object with the matching h.toX() helper.
+ final LALScriptModel.ValueAccessConditionValue vacv =
+ (LALScriptModel.ValueAccessConditionValue) cv;
+ final StringBuilder rhsBuf = new StringBuilder();
+ emitOperandRespectingCast(rhsBuf, vacv.getValue(),
vacv.getCastType(), genCtx);
+ final Class<?> rhsResolved = genCtx.lastResolvedType;
+ final ExprType target = primitiveToExprType(lhsType);
+ if (rhsResolved != null
+ && (rhsResolved == int.class || rhsResolved == long.class
+ || rhsResolved == float.class || rhsResolved ==
double.class)
+ && genCtx.lastRawChain != null) {
+ sb.append(widenPrimitiveExpr(genCtx.lastRawChain,
+ primitiveToExprType(rhsResolved), target));
+ } else {
Review Comment:
This branch drops the null-guard information carried in
`genCtx.lastNullChecks` for safe-navigated typed proto fields. If the RHS is
something like `parsed?.response?.responseCode?.value`, the generated
comparison reuses `lastRawChain` directly and can throw `NullPointerException`
instead of evaluating to `false` when an intermediate field is null.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -214,24 +388,122 @@ static void generateConditionValue(final StringBuilder
sb,
}
}
+ /**
+ * Render a numeric RHS for a comparison operator. The literal is emitted
+ * in a form compatible with the LHS resolved type so the comparison stays
+ * in the LHS's primitive space — no defensive widening to long.
+ *
+ * <p>Examples (LHS in parens):
+ * <ul>
+ * <li>(int) {@code < 10000} → {@code 10000}</li>
+ * <li>(long) {@code < 10000} → {@code 10000L}</li>
+ * <li>(double) {@code < 1.5} → {@code 1.5d}</li>
+ * <li>(int) {@code < 1.5} → {@code 1.5d} — JLS will widen LHS to
double</li>
+ * </ul>
+ */
static void generateConditionValueNumeric(
final StringBuilder sb,
final LALScriptModel.ConditionValue cv,
+ final Class<?> lhsType,
final LALClassGenerator.GenCtx genCtx) {
if (cv instanceof LALScriptModel.NumberConditionValue) {
- sb.append((long) ((LALScriptModel.NumberConditionValue) cv)
- .getValue()).append("L");
+ final LALScriptModel.NumberConditionValue ncv =
+ (LALScriptModel.NumberConditionValue) cv;
+ final String literal = ncv.getLiteral();
+ if (literal != null) {
+ sb.append(formatNumericLiteralForCompare(literal, lhsType));
+ } else {
+ // Synthesised value (no source text).
+ sb.append(formatDoubleAsLiteral(ncv.getValue(), lhsType));
+ }
} else if (cv instanceof LALScriptModel.ValueAccessConditionValue) {
- sb.append("h.toLong(");
- generateValueAccessObj(sb,
- ((LALScriptModel.ValueAccessConditionValue) cv).getValue(),
- null, genCtx);
- sb.append(")");
+ // RHS value access — emit the LHS-typed primitive form. If
+ // codegen managed to land it as a primitive (typed proto field,
+ // arithmetic), reuse lastRawChain widened to lhsType; otherwise
+ // wrap the boxed object with the matching h.toX() helper.
+ final LALScriptModel.ValueAccessConditionValue vacv =
+ (LALScriptModel.ValueAccessConditionValue) cv;
+ final StringBuilder rhsBuf = new StringBuilder();
+ emitOperandRespectingCast(rhsBuf, vacv.getValue(),
vacv.getCastType(), genCtx);
+ final Class<?> rhsResolved = genCtx.lastResolvedType;
+ final ExprType target = primitiveToExprType(lhsType);
+ if (rhsResolved != null
+ && (rhsResolved == int.class || rhsResolved == long.class
+ || rhsResolved == float.class || rhsResolved ==
double.class)
+ && genCtx.lastRawChain != null) {
+ sb.append(widenPrimitiveExpr(genCtx.lastRawChain,
+ primitiveToExprType(rhsResolved), target));
+ } else {
+ sb.append(wrapAsPrimitive(rhsBuf.toString(), target));
+ }
} else {
sb.append("0L");
}
}
+ private static String formatNumericLiteralForCompare(final String literal,
+ final Class<?>
lhsType) {
+ final NumericLiteral parsed = NumericLiteral.parse(literal);
+ // Honour the user's declared literal type when wider than LHS;
+ // otherwise emit in the LHS form so the compare stays in that space.
+ final ExprType lhsExpr = primitiveToExprType(lhsType);
+ final ExprType result = parsed.type.compareTo(lhsExpr) > 0 ?
parsed.type : lhsExpr;
+ if (result == parsed.type) {
+ return parsed.javaText;
+ }
+ // Widen literal to LHS type via suffix change (no Java cast needed for
+ // numeric literals — the widened form is itself a valid literal).
+ return widenLiteralToType(parsed.javaText, parsed.type, result);
+ }
+
+ private static String widenLiteralToType(final String text,
+ final ExprType from,
+ final ExprType to) {
+ // Strip trailing suffix (if any) and re-emit in `to`.
+ String body = text;
+ final char last = body.isEmpty() ? 0 : body.charAt(body.length() - 1);
+ if (last == 'L' || last == 'F' || last == 'f' || last == 'd' || last
== 'D') {
+ body = body.substring(0, body.length() - 1);
+ }
+ switch (to) {
+ case LONG:
+ return body + "L";
+ case FLOAT:
+ return (body.contains(".") ? body : body + ".0") + "f";
+ case DOUBLE:
+ return body.contains(".") ? body : body + ".0";
Review Comment:
Widening exponent-form literals by appending `.0` produces invalid Java. For
example, comparing a `Double` operand against `1e6f` reaches this path and
emits `1e6.0`, which won't compile. The exponent form needs to be preserved
when changing only the target type/suffix.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -865,80 +1173,421 @@ static void generateProcessRegistryCall(
// ==================== Utility methods ====================
+ // ==================== Binary expression codegen ====================
+
+ /**
+ * JLS-style numeric type tag used for binary numeric promotion. Ordered
+ * narrow → wide, so {@code commonType(a, b)} can return the wider of two.
+ */
+ private enum ExprType {
+ INT, LONG, FLOAT, DOUBLE,
+ STRING, BOOLEAN, OBJECT, UNKNOWN;
+
+ boolean isNumeric() {
+ return this == INT || this == LONG || this == FLOAT || this ==
DOUBLE;
+ }
+ }
+
+ /**
+ * Parsed representation of a NUMBER literal token. Carries the inferred
+ * Java type and the literal text trimmed/suffixed for direct emission as
+ * Java source.
+ */
+ private static final class NumericLiteral {
+ final ExprType type;
+ /** Java-source representation, e.g. "10000", "10000L", "1.5", "1.5f".
*/
+ final String javaText;
+
+ private NumericLiteral(final ExprType type, final String javaText) {
+ this.type = type;
+ this.javaText = javaText;
+ }
+
+ static NumericLiteral parse(final String numText) {
+ String t = numText;
+ char suffix = 0;
+ if (!t.isEmpty()) {
+ final char last = t.charAt(t.length() - 1);
+ if (last == 'L' || last == 'l' || last == 'F' || last == 'f'
+ || last == 'D' || last == 'd') {
+ suffix = last;
+ t = t.substring(0, t.length() - 1);
+ }
+ }
+ final boolean fractional = t.contains(".") || t.contains("e") ||
t.contains("E");
+ switch (suffix) {
+ case 'L':
+ case 'l':
+ return new NumericLiteral(ExprType.LONG, t + "L");
+ case 'F':
+ case 'f':
+ return new NumericLiteral(ExprType.FLOAT,
+ (fractional ? t : (t + ".0")) + "f");
+ case 'D':
+ case 'd':
+ return new NumericLiteral(ExprType.DOUBLE,
+ (fractional ? t : (t + ".0")) + "d");
+ default:
+ if (fractional) {
+ return new NumericLiteral(ExprType.DOUBLE, t);
+ }
+ // Bare integer literal — INT if it fits, else LONG.
+ try {
+ Integer.parseInt(t);
+ return new NumericLiteral(ExprType.INT, t);
+ } catch (NumberFormatException e) {
+ return new NumericLiteral(ExprType.LONG, t + "L");
+ }
+ }
+ }
+ }
+
/**
- * Returns {@code true} when every concat part is numeric — i.e. a
- * parenthesized expression with an {@code Integer} or {@code Long} cast,
- * or a bare number literal. If so, {@code +} is arithmetic, not string
- * concatenation.
+ * Returns the JLS-style binary numeric promotion result for the two
operand
+ * types. Both must be numeric; otherwise this is a programmer bug (the
+ * caller is responsible for the non-numeric error path).
*/
- private static boolean allPartsNumeric(final
List<LALScriptModel.ValueAccess> parts) {
- for (final LALScriptModel.ValueAccess part : parts) {
- if (part.isNumberLiteral()) {
+ private static ExprType promote(final ExprType a, final ExprType b) {
+ if (a == ExprType.DOUBLE || b == ExprType.DOUBLE) {
+ return ExprType.DOUBLE;
+ }
+ if (a == ExprType.FLOAT || b == ExprType.FLOAT) {
+ return ExprType.FLOAT;
+ }
+ if (a == ExprType.LONG || b == ExprType.LONG) {
+ return ExprType.LONG;
+ }
+ return ExprType.INT;
+ }
+
+ private static String javaPrimitiveName(final ExprType t) {
+ switch (t) {
+ case LONG:
+ return "long";
+ case FLOAT:
+ return "float";
+ case DOUBLE:
+ return "double";
+ case INT:
+ default:
+ return "int";
+ }
+ }
+
+ private static String javaWrapperName(final ExprType t) {
+ switch (t) {
+ case LONG:
+ return "Long";
+ case FLOAT:
+ return "Float";
+ case DOUBLE:
+ return "Double";
+ case INT:
+ default:
+ return "Integer";
+ }
+ }
+
+ private static Class<?> primitiveClass(final ExprType t) {
+ switch (t) {
+ case LONG:
+ return long.class;
+ case FLOAT:
+ return float.class;
+ case DOUBLE:
+ return double.class;
+ case INT:
+ default:
+ return int.class;
+ }
+ }
+
+ /**
+ * Compile-time inference of an operand's Java type. Operates on the same
+ * AST that codegen will subsequently emit, so the result is consistent.
+ */
+ private static ExprType inferType(final LALScriptModel.ValueAccess part,
+ final LALClassGenerator.GenCtx genCtx) {
+ if (part == null) {
+ return ExprType.UNKNOWN;
+ }
+ if (part.isStringLiteral()) {
+ return ExprType.STRING;
+ }
+ if (part.isNumberLiteral() && part.getChain().isEmpty()) {
+ return NumericLiteral.parse(part.getSegments().get(0)).type;
+ }
+ // (expr as Cast) primary — only trustable when no further chain is
applied.
+ if (part.getParenInner() != null && part.getChain().isEmpty()) {
+ if (part.getParenCast() != null) {
+ return castToType(part.getParenCast());
+ }
+ // Pure grouping (no cast): the parens just preserve precedence.
+ return inferType(part.getParenInner(), genCtx);
+ }
+ // Nested binary expression: recompute via its parts/ops.
+ if (!part.getConcatParts().isEmpty()) {
+ return inferBinaryType(part.getConcatParts(), part.getConcatOps(),
genCtx);
+ }
+ // def variable with a known resolved type.
+ if (!part.getSegments().isEmpty()) {
+ final LALClassGenerator.LocalVarInfo lv =
+ genCtx.localVars.get(part.getSegments().get(0));
+ if (lv != null && part.getChain().isEmpty()) {
+ final Class<?> t = lv.resolvedType;
+ if (t == int.class || t == Integer.class) {
+ return ExprType.INT;
+ }
+ if (t == long.class || t == Long.class) {
+ return ExprType.LONG;
+ }
+ if (t == float.class || t == Float.class) {
+ return ExprType.FLOAT;
+ }
+ if (t == double.class || t == Double.class) {
+ return ExprType.DOUBLE;
+ }
+ if (t == String.class) {
+ return ExprType.STRING;
+ }
+ if (t == boolean.class || t == Boolean.class) {
+ return ExprType.BOOLEAN;
+ }
+ return ExprType.OBJECT;
+ }
+ }
+ // tag(), sourceAttribute(), parsed.* without typed input — String.
+ if ("tag".equals(part.getFunctionCallName())
+ || "sourceAttribute".equals(part.getFunctionCallName())) {
+ return ExprType.STRING;
+ }
+ return ExprType.UNKNOWN;
Review Comment:
`inferType()` still treats all `parsed.*` accesses as `UNKNOWN`, so uncast
typed proto numerics never participate in arithmetic inference. For example,
`parsed?.response?.responseCode?.value + 1` will fall back to string
concatenation, and `-`/`*`/`/` on the same field will be rejected, even though
the underlying proto getter is numeric.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
