Copilot commented on code in PR #13858:
URL: https://github.com/apache/skywalking/pull/13858#discussion_r3178152264
##########
oap-server/analyzer/log-analyzer/src/main/antlr4/org/apache/skywalking/lal/rt/grammar/LALLexer.g4:
##########
@@ -111,8 +113,17 @@ TRUE: 'true';
FALSE: 'false';
NULL: 'null';
+// Numeric literal. Suffix rules mirror Java:
+// * `L` / `l` is valid only on integer literals (no decimal, no exponent).
+// * `F` / `f` and `D` / `d` are valid on any form.
+// Bare integer literals (no suffix, no decimal, no exponent) are interpreted
as int
+// by the compiler if they fit, otherwise long. Bare decimal/exponent literals
are
+// double unless suffixed.
NUMBER
- : Digit+ ('.' Digit+)?
+ : Digit+ '.' Digit+ ([eE] [+\-]? Digit+)? [FfDd]?
+ | Digit+ [eE] [+\-]? Digit+ [FfDd]?
+ | '.' Digit+ ([eE] [+\-]? Digit+)? [FfDd]?
+ | Digit+ [LlFfDd]?
Review Comment:
Widening the shared `NUMBER` token here also changes every integer-only slot
that reuses it, such as `rateLimit { rpm ... }` and `[index]` access. Inputs
like `rpm 1L` or `parsed.items[1L]` now lex successfully, but the parser still
feeds them to `Long.parseLong(...)` / `Integer.parseInt(...)`, so they fail
later with an uncaught `NumberFormatException` instead of a normal
syntax/validation error.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALDefCodegen.java:
##########
@@ -169,6 +169,10 @@ private static Class<?> resolveDefCastType(final String
castType) {
return Long.class;
case "Integer":
return Integer.class;
+ case "Double":
+ return Double.class;
+ case "Float":
+ return Float.class;
Review Comment:
Adding `Double`/`Float` here makes `def x = tag("a") as Double/Float` parse,
but def assignments still use a plain Java cast in `generateDefStatement`
instead of `h.toDouble()` / `h.toFloat()`. Since `tag()` and other common
sources return `String`, these new defs compile but will fail at runtime with
`ClassCastException` instead of performing the requested numeric conversion.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -865,80 +1205,479 @@ 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 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 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() — always return String at runtime.
+ if ("tag".equals(part.getFunctionCallName())
+ || "sourceAttribute".equals(part.getFunctionCallName())) {
+ return ExprType.STRING;
+ }
+ // parsed.* — when the rule has a typed inputType (no JSON/YAML/TEXT
+ // parser), walk the proto getter chain via reflection so primitive
+ // numeric leaves participate in arithmetic and don't fall through
+ // to string concat. Mirrors {@link #generateExtraLogAccess} but
+ // does not emit code or local variables.
+ if (part.isParsedRef()
+ && genCtx.parserType == LALClassGenerator.ParserType.NONE
+ && genCtx.inputType != null) {
+ final Class<?> primitive = walkProtoChainPrimitiveType(
+ part.getChain(), genCtx.inputType);
+ if (primitive != null) {
+ if (primitive == int.class) {
+ return ExprType.INT;
+ }
+ if (primitive == long.class) {
+ return ExprType.LONG;
+ }
+ if (primitive == float.class) {
+ return ExprType.FLOAT;
+ }
+ if (primitive == double.class) {
+ return ExprType.DOUBLE;
+ }
+ if (primitive == boolean.class) {
+ return ExprType.BOOLEAN;
+ }
+ }
+ }
+ return ExprType.UNKNOWN;
+ }
+
+ /**
+ * Walk a {@code parsed.*} field-segment chain over the typed proto root
+ * via reflection, returning the primitive return type of the final getter
+ * if the chain is field-only and ends at a primitive accessor; {@code
null}
+ * otherwise.
+ */
+ private static Class<?> walkProtoChainPrimitiveType(
+ final List<LALScriptModel.ValueAccessSegment> chain,
+ final Class<?> rootType) {
+ if (chain == null || chain.isEmpty()) {
+ return null;
+ }
+ Class<?> currentType = rootType;
+ for (int i = 0; i < chain.size(); i++) {
+ final LALScriptModel.ValueAccessSegment seg = chain.get(i);
+ if (!(seg instanceof LALScriptModel.FieldSegment)) {
+ return null;
+ }
+ final String field = ((LALScriptModel.FieldSegment) seg).getName();
+ final String getter = "get" +
Character.toUpperCase(field.charAt(0))
+ + field.substring(1);
+ try {
+ currentType = currentType.getMethod(getter).getReturnType();
+ } catch (NoSuchMethodException e) {
+ return null;
+ }
+ }
+ return currentType.isPrimitive() ? currentType : null;
+ }
+
+ private static ExprType castToType(final String cast) {
+ if ("Integer".equals(cast)) {
+ return ExprType.INT;
+ }
+ if ("Long".equals(cast)) {
+ return ExprType.LONG;
+ }
+ if ("Float".equals(cast)) {
+ return ExprType.FLOAT;
+ }
+ if ("Double".equals(cast)) {
+ return ExprType.DOUBLE;
+ }
+ if ("String".equals(cast)) {
+ return ExprType.STRING;
+ }
+ if ("Boolean".equals(cast)) {
+ return ExprType.BOOLEAN;
+ }
+ return ExprType.UNKNOWN;
+ }
+
/**
- * 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.
+ * Walk the operand list applying JLS promotion left-to-right, treating any
+ * {@code +} that touches a String operand as string concat (which sticks).
*/
- private static boolean allPartsNumeric(final
List<LALScriptModel.ValueAccess> parts) {
- for (final LALScriptModel.ValueAccess part : parts) {
- if (part.isNumberLiteral()) {
+ private static ExprType inferBinaryType(
+ final List<LALScriptModel.ValueAccess> parts,
+ final List<LALScriptModel.BinaryOp> ops,
+ final LALClassGenerator.GenCtx genCtx) {
+ ExprType acc = inferType(parts.get(0), genCtx);
+ for (int i = 1; i < parts.size(); i++) {
+ final ExprType rhs = inferType(parts.get(i), genCtx);
+ final LALScriptModel.BinaryOp op = ops.get(i - 1);
+ if (op == LALScriptModel.BinaryOp.PLUS
+ && (acc == ExprType.STRING || rhs == ExprType.STRING)) {
+ acc = ExprType.STRING;
+ continue;
+ }
+ if (acc.isNumeric() && rhs.isNumeric()) {
+ acc = promote(acc, rhs);
continue;
}
- if (part.getParenInner() != null &&
isNumericCast(part.getParenCast())) {
+ // Anything else for + (object + object) → fallback to string
concat
+ // (preserves long-standing behaviour for `tag("a") + tag("b")`
etc.).
+ if (op == LALScriptModel.BinaryOp.PLUS) {
+ acc = ExprType.STRING;
Review Comment:
This flattening changes Java `+` semantics once a later operand is a
`String`. For example, `1 + 2 + "x"` is inferred as `STRING` here and then
emitted as `"" + 1 + 2 + "x"`, which evaluates to `"12x"` instead of the
correct `"3x"`. Any add-chain with numeric terms before the first string
operand will be miscompiled the same way.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -163,29 +165,227 @@ static void generateNumericComparison(
final LALScriptModel.ComparisonCondition cc,
final String op,
final LALClassGenerator.GenCtx genCtx) {
- // Generate left side into buffer to inspect resolved type
+ // Decide the primitive comparison type from explicit casts and the
+ // operand shapes — both sides influence the choice via JLS-style
+ // promotion. RHS-side cast is read off ValueAccessConditionValue;
+ // RHS literals contribute via their inferred numeric type.
+ final ExprType lhsType = inferComparisonOperandType(cc.getLeft(),
cc.getLeftCast(), genCtx);
+ final ExprType rhsType = inferRightHandType(cc.getRight(), genCtx);
+ final ExprType cmpType = pickComparisonType(lhsType, rhsType);
+ final Class<?> cmpClass = primitiveClass(cmpType);
+
+ // Render the LHS, then capture its primitive metadata BEFORE the RHS
+ // generator overwrites genCtx fields. A top-level numeric cast on
+ // the comparison (`tag("a") as Integer < 1.5`) is honoured first so
+ // the operand is rendered in the user's declared primitive, not in
+ // the promoted comparison type.
+ genCtx.lastNullChecks = null;
final StringBuilder leftBuf = new StringBuilder();
- generateValueAccessObj(leftBuf, cc.getLeft(), null, genCtx);
-
- final boolean primitiveNumeric = genCtx.lastResolvedType != null
- && (genCtx.lastResolvedType == int.class
- || genCtx.lastResolvedType == long.class);
-
- if (primitiveNumeric && genCtx.lastRawChain != null) {
- // Direct primitive comparison — no boxing, no h.toLong()
- if (genCtx.lastNullChecks != null) {
- sb.append("(").append(genCtx.lastNullChecks).append(" ? false
: ")
- .append(genCtx.lastRawChain).append(op);
- generateConditionValueNumeric(sb, cc.getRight(), genCtx);
- sb.append(")");
- } else {
- sb.append(genCtx.lastRawChain).append(op);
- generateConditionValueNumeric(sb, cc.getRight(), genCtx);
- }
+ emitOperandRespectingCast(leftBuf, cc.getLeft(), cc.getLeftCast(),
genCtx);
+ final Class<?> lhsResolved = genCtx.lastResolvedType;
+ final String lhsRawChain = genCtx.lastRawChain;
+ final String lhsNullChecks = genCtx.lastNullChecks;
+ final boolean lhsPrimitive = lhsResolved != null
+ && (lhsResolved == int.class || lhsResolved == long.class
+ || lhsResolved == float.class || lhsResolved == double.class);
+
+ final String leftExpr;
+ if (lhsPrimitive && lhsRawChain != null) {
+ // Already primitive — widen to cmpType only if narrower.
+ leftExpr = widenPrimitiveExpr(lhsRawChain,
+ primitiveToExprType(lhsResolved), cmpType);
} else {
- // Fallback: h.toLong() conversion
- sb.append("h.toLong(").append(leftBuf).append(")").append(op);
- generateConditionValueNumeric(sb, cc.getRight(), genCtx);
+ // Untyped operand — wrap with the helper that produces the
+ // chosen primitive type. h.toLong is no longer the universal
+ // fallback when the user wrote `as Double` / `as Float`.
+ leftExpr = wrapAsPrimitive(leftBuf.toString(), cmpType);
+ }
+
+ // Render the RHS into its own buffer so we can read back any null
+ // guards it produced (e.g. from a typed-proto chain like
+ // `parsed?.response?.responseCode?.value`) and apply them at the
+ // outer ternary alongside the LHS guards.
+ genCtx.lastNullChecks = null;
+ final StringBuilder rhsBuf = new StringBuilder();
+ generateConditionValueNumeric(rhsBuf, cc.getRight(), cmpClass, genCtx);
+ final String rhsNullChecks = genCtx.lastNullChecks;
+
+ final String combinedNullChecks = combineNullChecks(lhsNullChecks,
rhsNullChecks);
+ if (combinedNullChecks != null) {
+ sb.append("(").append(combinedNullChecks).append(" ? false : ")
+ .append(leftExpr).append(op).append(rhsBuf).append(")");
+ } else {
+ sb.append(leftExpr).append(op).append(rhsBuf);
+ }
+ }
+
+ /**
+ * Combine LHS / RHS null-guard expressions for a comparison. Either side
+ * may be {@code null} (no guard); the result is the disjunction so the
+ * comparison short-circuits to {@code false} as soon as any participant
+ * is null, mirroring the previous single-side behaviour.
+ */
+ private static String combineNullChecks(final String lhs, final String
rhs) {
+ if (lhs == null && rhs == null) {
+ return null;
+ }
+ if (lhs == null) {
+ return rhs;
+ }
+ if (rhs == null) {
+ return lhs;
+ }
+ return lhs + " || " + rhs;
+ }
+
+ /**
+ * Read the numeric type the user declared for the left side of a
+ * comparison: top-level {@code as} cast on the comparison wins, otherwise
+ * fall back to inspecting the operand AST.
+ */
+ private static ExprType inferComparisonOperandType(
+ final LALScriptModel.ValueAccess value,
+ final String topLevelCast,
+ final LALClassGenerator.GenCtx genCtx) {
+ final ExprType fromCast = castToType(topLevelCast);
+ if (fromCast.isNumeric()) {
+ return fromCast;
+ }
+ final ExprType inferred = inferType(value, genCtx);
+ return inferred.isNumeric() ? inferred : ExprType.LONG;
+ }
+
+ /**
+ * Read the numeric type of a comparison's RHS — literal type for number
+ * literals, declared cast for value accesses, otherwise unknown.
+ */
+ private static ExprType inferRightHandType(
+ final LALScriptModel.ConditionValue cv,
+ final LALClassGenerator.GenCtx genCtx) {
+ if (cv instanceof LALScriptModel.NumberConditionValue) {
+ final String literal = ((LALScriptModel.NumberConditionValue)
cv).getLiteral();
+ return literal != null ? NumericLiteral.parse(literal).type :
ExprType.LONG;
+ }
+ if (cv instanceof LALScriptModel.ValueAccessConditionValue) {
+ final LALScriptModel.ValueAccessConditionValue vacv =
+ (LALScriptModel.ValueAccessConditionValue) cv;
+ final ExprType castT = castToType(vacv.getCastType());
+ if (castT.isNumeric()) {
+ return castT;
+ }
+ return inferType(vacv.getValue(), genCtx);
+ }
+ return ExprType.UNKNOWN;
+ }
+
+ /**
+ * Pick the JLS-promoted primitive type for a comparison given both sides'
+ * inferred types. Numeric sides drive promotion; non-numeric sides default
+ * to {@code LONG} so the comparison still has a primitive home.
+ */
+ private static ExprType pickComparisonType(final ExprType lhs, final
ExprType rhs) {
+ if (lhs.isNumeric() && rhs.isNumeric()) {
+ return promote(lhs, rhs);
+ }
+ if (lhs.isNumeric()) {
+ return lhs;
+ }
+ if (rhs.isNumeric()) {
+ return rhs;
+ }
+ return ExprType.LONG;
+ }
+
+ /**
+ * Emit a primitive widening cast on an already-primitive expression, only
+ * when the source type is narrower than the target.
+ */
+ private static String widenPrimitiveExpr(final String expr,
+ final ExprType from,
+ final ExprType to) {
+ if (from == to || promote(from, to) == from) {
+ return expr;
+ }
+ return "(" + javaPrimitiveName(to) + ") " + expr;
+ }
+
+ /**
+ * Render an operand for a numeric comparison while preserving the user's
+ * declared cast. Two-phase to avoid double-coercion:
+ * <ol>
+ * <li>Render the operand expression. Typed proto fields, arithmetic
+ * results, and paren-cast operands already land as a primitive and
+ * record the type via {@code genCtx.lastResolvedType}.</li>
+ * <li>If the rendered expression is non-primitive (a boxed/Object)
+ * and the user declared a numeric cast, wrap it with the matching
+ * {@code h.toX()} helper so the primitive identity is preserved
+ * — e.g. {@code tag("a") as Integer < 1.5} renders
+ * {@code (double) h.toInt(h.tagValue("a")) < 1.5}, not
+ * {@code h.toDouble(h.tagValue("a")) < 1.5}.</li>
+ * </ol>
+ */
+ private static void emitOperandRespectingCast(
+ final StringBuilder sb,
+ final LALScriptModel.ValueAccess value,
+ final String castType,
+ final LALClassGenerator.GenCtx genCtx) {
+ final int start = sb.length();
+ generateValueAccessObj(sb, value, castType, genCtx);
+ final Class<?> resolved = genCtx.lastResolvedType;
+ final boolean alreadyPrimitive = resolved != null &&
resolved.isPrimitive();
+ if (alreadyPrimitive) {
+ // Native primitive — the declared cast is either redundant
+ // (matches) or will be honoured by JLS widening at the
+ // comparison site. Leave the rendered expression as-is.
+ return;
Review Comment:
This early return skips an explicit top-level numeric cast whenever the
operand already produced a primitive/raw chain. That means casts on the newly
supported arithmetic expressions are ignored: for example, `if ((tag("a") as
Long) + 1 as Integer < 10)` will still compare the raw `long` sum instead of
first coercing it to `int` as the user wrote.
--
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]
