You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "wankai123 (via GitHub)" <gi...@apache.org> on 2026/04/30 09:06:57 UTC
[PR] Fix: LAL compiler treated `(tag("x") as Integer) + (tag("y") as Integer)` as string concatenation instead of numeric addition. [skywalking]
wankai123 opened a new pull request, #13857:
URL: https://github.com/apache/skywalking/pull/13857
- [X] Add a unit test to verify that the fix works.
- [ ] Explain briefly why the bug exists and how to fix it.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [X] Update the [`CHANGES` log](https://github.com/apache/skywalking/blob/master/docs/en/changes/changes.md).
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: LAL compiler treated `(tag("x") as Integer) + (tag("y") as Integer)` as string concatenation instead of numeric addition. [skywalking]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13857:
URL: https://github.com/apache/skywalking/pull/13857#discussion_r3166857178
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -45,6 +45,7 @@
* def variable myVar?.getAsString() _def_myVar?.getAsString()
* Parenthesized (expr as String).trim() h.toStr(...).trim()
* String concat "${log.service}:${parsed.code}" "" + ... + ":" + ...
+ * Arithmetic sum (tag("a") as Integer) + (tag("b") as Integer) (h.toInt(...) + h.toInt(...))
Review Comment:
The class-level JavaDoc example for "Arithmetic sum" doesn’t match the new generated output: the implementation now emits `Long.valueOf(...)` with explicit `(long)` promotion for Integer parts. Updating the example to reflect the actual generated code will prevent confusion when debugging codegen output.
```suggestion
* Arithmetic sum (tag("a") as Integer) + (tag("b") as Integer) Long.valueOf((long) h.toInt(...) + (long) h.toInt(...))
```
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -858,6 +865,82 @@ static void generateProcessRegistryCall(
// ==================== Utility methods ====================
+ /**
+ * 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.
+ */
+ private static boolean allPartsNumeric(final List<LALScriptModel.ValueAccess> parts) {
+ for (final LALScriptModel.ValueAccess part : parts) {
+ if (part.isNumberLiteral()) {
+ continue;
+ }
+ if (part.getParenInner() != null && isNumericCast(part.getParenCast())) {
+ continue;
+ }
+ return false;
+ }
+ return true;
+ }
+
+ private static boolean isNumericCast(final String cast) {
+ return "Integer".equals(cast) || "Long".equals(cast);
+ }
+
+ /**
+ * Generates an arithmetic sum expression for a list of numeric parts.
+ * Always uses {@code long} arithmetic to avoid Javassist autoboxing
+ * restrictions (Javassist cannot pass a primitive {@code int/long} to a
+ * method that expects {@code Object}, e.g. {@code h.toLong(int)}).
+ *
+ * <p>Two outputs are produced:
+ * <ul>
+ * <li>The raw {@code long} expression is stored in
+ * {@code genCtx.lastRawChain} so that {@code generateNumericComparison}
+ * can emit a direct primitive comparison without a
+ * {@code h.toLong()} wrapper.</li>
+ * <li>{@code Long.valueOf(rawExpr)} is appended to {@code sb} so that
+ * non-comparison contexts (tag assignment, {@code h.toStr()}, etc.)
+ * receive a boxed {@code Long} — a valid {@code Object}.</li>
+ * </ul>
+ *
+ * <p>Examples:
+ * <pre>{@code
+ * (tag("a") as Integer) + (tag("b") as Integer) < 10000
+ * rawExpr → ((long) h.toInt(h.tagValue("a")) + (long) h.toInt(h.tagValue("b")))
+ * in sb → Long.valueOf(((long) h.toInt(...) + (long) h.toInt(...)))
+ * comparison emits → rawExpr < 10000L (via lastRawChain / primitiveNumeric path)
+ * }</pre>
+ */
+ private static void generateArithmeticSum(final StringBuilder sb,
+ final List<LALScriptModel.ValueAccess> parts,
+ final LALClassGenerator.GenCtx genCtx) {
+ final StringBuilder expr = new StringBuilder("(");
+ for (int i = 0; i < parts.size(); i++) {
+ if (i > 0) {
+ expr.append(" + ");
+ }
+ final LALScriptModel.ValueAccess part = parts.get(i);
+ if (part.isNumberLiteral()) {
+ expr.append(part.getSegments().get(0)).append("L");
+ } else if ("Long".equals(part.getParenCast())) {
Review Comment:
In `generateArithmeticSum(...)`, number literals are emitted by appending `"L"` to the raw token (`part.getSegments().get(0)`). If the DSL literal includes a decimal (e.g. `1.5`) this produces invalid Java (`1.5L`). Even if you keep arithmetic sums limited to integer math, it would be safer to either reject non-integer numeric literals here (compile-time error) or route them to a separate double-arithmetic path instead of emitting an invalid suffix.
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -858,6 +865,82 @@ static void generateProcessRegistryCall(
// ==================== Utility methods ====================
+ /**
+ * 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.
+ */
+ private static boolean allPartsNumeric(final List<LALScriptModel.ValueAccess> parts) {
+ for (final LALScriptModel.ValueAccess part : parts) {
+ if (part.isNumberLiteral()) {
+ continue;
+ }
+ if (part.getParenInner() != null && isNumericCast(part.getParenCast())) {
+ continue;
+ }
+ return false;
+ }
Review Comment:
`allPartsNumeric(...)` treats any `numberLiteral` as numeric without validating that it's an integer literal and without ensuring the term has no chained segments. This can miscompile expressions like `1.5 + 2.5` (would emit `1.5L`, which is invalid Java) or `(x as Integer).toString() + ...` (the `.toString()` chain would be dropped when generating the arithmetic sum). Consider restricting numeric literals to integer-only (no decimal/exponent) and requiring `part.getChain().isEmpty()` for both numeric literals and paren-cast parts before selecting the arithmetic-sum path; otherwise fall back to string concatenation.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: LAL compiler treated `(tag("x") as Integer) + (tag("y") as Integer)` as string concatenation instead of numeric addition. [skywalking]
Posted by "wankai123 (via GitHub)" <gi...@apache.org>.
wankai123 merged PR #13857:
URL: https://github.com/apache/skywalking/pull/13857
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix: LAL compiler treated `(tag("x") as Integer) + (tag("y") as Integer)` as string concatenation instead of numeric addition. [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng commented on code in PR #13857:
URL: https://github.com/apache/skywalking/pull/13857#discussion_r3167039907
##########
oap-server/analyzer/log-analyzer/src/main/java/org/apache/skywalking/oap/log/analyzer/v2/compiler/LALValueCodegen.java:
##########
@@ -858,6 +865,82 @@ static void generateProcessRegistryCall(
// ==================== Utility methods ====================
+ /**
+ * 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.
+ */
+ private static boolean allPartsNumeric(final List<LALScriptModel.ValueAccess> parts) {
+ for (final LALScriptModel.ValueAccess part : parts) {
+ if (part.isNumberLiteral()) {
+ continue;
+ }
+ if (part.getParenInner() != null && isNumericCast(part.getParenCast())) {
+ continue;
+ }
+ return false;
+ }
Review Comment:
I will fully investage this in another PR.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org