You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "wu-sheng (via GitHub)" <gi...@apache.org> on 2026/03/26 12:52:10 UTC
[PR] Refactor MAL codegen into focused classes, fix CI issues [skywalking]
wu-sheng opened a new pull request, #13763:
URL: https://github.com/apache/skywalking/pull/13763
### Refactor MAL codegen + CI fixes
- [x] If this is non-trivial feature, paste the links/URLs to the design doc.
- [x] Tests(including UT, IT, E2E) are added to verify the new feature.
**1. Refactor MALClassGenerator (1484 → 5 focused classes)**
Pure refactoring, no behavior change. Split by responsibility:
| Class | Lines | Responsibility |
|---|---|---|
| MALClassGenerator | 374 | Public API + orchestration |
| MALExprCodegen | 492 | Expression → Java source (statement + inline) |
| MALMethodChainCodegen | 366 | Method chain + extension + argument codegen |
| MALMetadataExtractor | 371 | Static AST → metadata extraction |
| MALBytecodeHelper | 293 | Javassist class naming, debug, bytecode attrs |
All 79 unit tests and 1268 v1-v2 comparison tests pass unchanged.
**2. Fix upload-artifact v4 conflict**
`upload-artifact@v4` rejects re-uploads with the same artifact name.
When matrix jobs retry on failure, the name collides (HTTP 409 Conflict).
Added `overwrite: true` to all failure-log upload steps.
**3. Fix Node.js 20 deprecation warning**
Set `FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true` globally and upgrade
`actions/checkout` from v4 to v6. Other actions don't have v6 yet
but the env var forces Node.js 24.
- [ ] If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #<issue number>.
- [ ] 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] Refactor MAL codegen into focused classes, fix CI issues [skywalking]
Posted by "wu-sheng (via GitHub)" <gi...@apache.org>.
wu-sheng merged PR #13763:
URL: https://github.com/apache/skywalking/pull/13763
--
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] Refactor MAL codegen into focused classes, fix CI issues [skywalking]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #13763:
URL: https://github.com/apache/skywalking/pull/13763#discussion_r2994702124
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALMethodChainCodegen.java:
##########
@@ -0,0 +1,366 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.oap.meter.analyzer.v2.compiler;
+
+import java.util.List;
+import org.apache.skywalking.oap.meter.analyzer.v2.compiler.rt.MalExtensionRegistry;
+
+/**
+ * Generates Java source for method chain calls on {@code SampleFamily}.
+ *
+ * <p>Handles two kinds of method calls in the chain:
+ * <ul>
+ * <li><b>Built-in methods</b>: {@code .sum(['tag'])}, {@code .rate("PT1M")}, {@code .tag({...})}
+ * — generates direct calls on the SampleFamily variable:
+ * {@code sf = sf.sum(Arrays.asList(new String[]{"tag"}));}</li>
+ * <li><b>Extension methods</b>: {@code .genai::estimateCost()}, {@code .test::scale(2.0)}
+ * — generates direct static method calls via SPI registry:
+ * {@code sf = TestMalExtension.scale(sf, 2.0);}</li>
+ * </ul>
+ *
+ * <p>Also handles all argument codegen: strings, numbers, string lists, number lists,
+ * enums, booleans, closures, and typed arguments for extension functions.
+ */
+final class MALMethodChainCodegen {
+
+ private final MALExprCodegen exprCodegen;
+
+ MALMethodChainCodegen(final MALExprCodegen exprCodegen) {
+ this.exprCodegen = exprCodegen;
+ }
+
+ // ==================== Statement-based chain codegen ====================
+
+ /**
+ * Emits each method chain call as a reassignment of {@code sf}.
+ *
+ * <p>For {@code metric.sum(['svc']).rate("PT1M")}, generates:
+ * <pre>
+ * sf = sf.sum(Arrays.asList(new String[]{"svc"}));
+ * sf = sf.rate("PT1M");
+ * </pre>
+ *
+ * <p>For extension calls like {@code .test::scale(2.0)}, generates:
+ * <pre>
+ * sf = TestMalExtension.scale(sf, 2.0);
+ * </pre>
+ */
+ void emitChainStatements(final StringBuilder sb,
+ final List<MALExpressionModel.MethodCall> chain) {
+ for (final MALExpressionModel.MethodCall mc : chain) {
+ if (mc.isExtension()) {
+ emitExtensionCall(sb, mc);
+ continue;
+ }
+ sb.append(" ").append(MALCodegenHelper.RUN_VAR).append(" = ")
+ .append(MALCodegenHelper.RUN_VAR).append('.').append(mc.getName()).append('(');
+ final List<MALExpressionModel.Argument> args = mc.getArguments();
+ if (MALCodegenHelper.VARARGS_STRING_METHODS.contains(mc.getName())
+ && !args.isEmpty() && allStringArgs(args)) {
+ sb.append("new String[]{");
+ for (int i = 0; i < args.size(); i++) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ generateArgument(sb, args.get(i));
+ }
+ sb.append('}');
+ } else {
+ final boolean primitiveDouble =
+ MALCodegenHelper.PRIMITIVE_DOUBLE_METHODS.contains(mc.getName());
+ for (int i = 0; i < args.size(); i++) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ generateMethodCallArg(sb, args.get(i), primitiveDouble);
+ }
+ }
+ sb.append(");\n");
+ }
+ }
+
+ // ==================== Inline chain codegen ====================
+
+ /**
+ * Emits method chain inline (for sub-expressions in binary ops and function call args).
+ *
+ * <p>For {@code metric.sum(['svc'])}, emits: {@code .sum(Arrays.asList(new String[]{"svc"}))}
+ */
+ void emitMethodChainInline(final StringBuilder sb,
+ final List<MALExpressionModel.MethodCall> chain) {
+ for (final MALExpressionModel.MethodCall mc : chain) {
Review Comment:
emitMethodChainInline() doesn't handle extension calls (namespace::method). If a chain contains an extension MethodCall, this will emit '.<method>(...)' instead of a static call via MalExtensionRegistry, which can produce invalid Java source when an extension call appears inside an expression argument (generateArgument -> exprCodegen.generateExprInline). Consider adding an mc.isExtension() branch here (inline-safe static-call codegen) or failing fast with a clear exception if inline extensions are intentionally unsupported.
```suggestion
for (final MALExpressionModel.MethodCall mc : chain) {
if (mc.isExtension()) {
throw new IllegalStateException(
"Inline extension method calls are not supported: " + mc.getName()
);
}
```
##########
oap-server/analyzer/meter-analyzer/src/main/java/org/apache/skywalking/oap/meter/analyzer/v2/compiler/MALExprCodegen.java:
##########
@@ -0,0 +1,492 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.skywalking.oap.meter.analyzer.v2.compiler;
+
+import java.util.List;
+
+/**
+ * Generates Java source for the {@code run(Map)} method body from a MAL expression AST.
+ *
+ * <p>Supports two codegen modes:
+ * <ul>
+ * <li><b>Statement-based</b> (for the top-level {@code run()} method): each expression
+ * step is a separate {@code sf = ...;} statement. Used by {@code generateRunMethod()}.
+ * Example for {@code metric.sum(['svc']).rate("PT1M")}:
+ * <pre>
+ * sf = ((SampleFamily) samples.getOrDefault("metric", SampleFamily.EMPTY));
+ * sf = sf.sum(Arrays.asList(new String[]{"svc"}));
+ * sf = sf.rate("PT1M");
+ * </pre></li>
+ * <li><b>Inline</b> (for sub-expressions in binary ops, function args): the entire
+ * expression is a single chained Java expression. Used by {@code generateExprInline()}.
+ * Example for {@code metric.sum(['svc'])} as a sub-expression:
+ * {@code ((SampleFamily) samples.getOrDefault("metric", SampleFamily.EMPTY)).sum(...)}</li>
+ * </ul>
+ *
+ * <p>Delegates method chain codegen to {@link MALMethodChainCodegen}.
+ */
+final class MALExprCodegen {
+
+ private static final String SF = MALCodegenHelper.SF;
+ private static final String RUN_VAR = MALCodegenHelper.RUN_VAR;
+
+ private final List<String> closureFieldNames;
+ private final MALMethodChainCodegen chainCodegen;
+ private int closureFieldIndex;
+ private int runTempCounter;
+
+ MALExprCodegen(final List<String> closureFieldNames) {
+ this.closureFieldNames = closureFieldNames;
+ this.chainCodegen = new MALMethodChainCodegen(this);
+ }
+
+ int getRunTempCounter() {
+ return runTempCounter;
+ }
+
+ /**
+ * Returns the next closure field name (e.g. {@code _tag}, {@code _forEach}).
+ * Called by {@link MALMethodChainCodegen} when generating closure arguments.
+ */
+ String nextClosureFieldName() {
+ return closureFieldNames.get(closureFieldIndex++);
+ }
+
+ // ==================== Statement-based codegen ====================
+
+ /**
+ * Generates the complete {@code run(Map)} method source.
+ *
+ * <p>For {@code metric.sum(['svc']).rate("PT1M")}, generates:
+ * <pre>
+ * public SampleFamily run(Map samples) {
+ * SampleFamily sf;
+ * sf = ((SampleFamily) samples.getOrDefault("metric", SampleFamily.EMPTY));
+ * sf = sf.sum(Arrays.asList(new String[]{"svc"}));
+ * sf = sf.rate("PT1M");
+ * return sf;
+ * }
+ * </pre>
+ */
+ String generateRunMethod(final MALExpressionModel.Expr ast) {
+ runTempCounter = 0;
+ closureFieldIndex = 0;
+ final StringBuilder sb = new StringBuilder();
+ sb.append("public ").append(SF).append(" run(java.util.Map samples) {\n");
+ sb.append(" ").append(SF).append(" ").append(RUN_VAR).append(";\n");
+ generateExprStatements(sb, ast);
+ sb.append(" return ").append(RUN_VAR).append(";\n");
+ sb.append("}\n");
+ return sb.toString();
+ }
+
+ private String nextTemp() {
+ return "_t" + runTempCounter++;
+ }
+
+ /**
+ * Emits the expression as a series of {@code sf = ...;} reassignment statements.
+ *
+ * <p>Dispatches by AST node type:
+ * <ul>
+ * <li>{@code metric} → lookup from samples map + chain calls</li>
+ * <li>{@code 100} → {@code sf = SampleFamily.EMPTY.plus(Long.valueOf(100L));}</li>
+ * <li>{@code metric1 + metric2} → temp variable for left, compute right, combine</li>
+ * <li>{@code -metric} → {@code sf = sf.negative();}</li>
+ * <li>{@code count(metric, [...])} → function call + chain</li>
+ * <li>{@code (metric * 2).sum([...])} → parenthesized + chain</li>
+ * </ul>
+ */
+ private void generateExprStatements(final StringBuilder sb,
+ final MALExpressionModel.Expr expr) {
+ if (expr instanceof MALExpressionModel.MetricExpr) {
+ generateMetricExprStatements(
+ sb, (MALExpressionModel.MetricExpr) expr);
+ } else if (expr instanceof MALExpressionModel.NumberExpr) {
+ final double val = ((MALExpressionModel.NumberExpr) expr).getValue();
+ sb.append(" ").append(RUN_VAR).append(" = ")
+ .append(SF).append(".EMPTY.plus(");
+ MALCodegenHelper.emitNumberValueOf(sb, val);
+ sb.append(");\n");
+ } else if (expr instanceof MALExpressionModel.BinaryExpr) {
+ generateBinaryExprStatements(
+ sb, (MALExpressionModel.BinaryExpr) expr);
+ } else if (expr instanceof MALExpressionModel.UnaryNegExpr) {
+ generateExprStatements(
+ sb, ((MALExpressionModel.UnaryNegExpr) expr).getOperand());
+ sb.append(" ").append(RUN_VAR).append(" = ")
+ .append(RUN_VAR).append(".negative();\n");
+ } else if (expr instanceof MALExpressionModel.FunctionCallExpr) {
+ generateFunctionCallStatements(
+ sb, (MALExpressionModel.FunctionCallExpr) expr);
+ } else if (expr instanceof MALExpressionModel.ParenChainExpr) {
+ generateParenChainStatements(
+ sb, (MALExpressionModel.ParenChainExpr) expr);
+ } else {
+ throw new IllegalArgumentException("Unknown expr type: " + expr);
+ }
+ }
+
+ /**
+ * Generates statements for {@code metric.sum(['svc']).rate("PT1M")}.
+ *
+ * <pre>
+ * sf = ((SampleFamily) samples.getOrDefault("metric", SampleFamily.EMPTY));
+ * sf = sf.sum(...);
+ * sf = sf.rate("PT1M");
+ * </pre>
+ */
+ private void generateMetricExprStatements(
+ final StringBuilder sb,
+ final MALExpressionModel.MetricExpr expr) {
+ sb.append(" ").append(RUN_VAR).append(" = ((").append(SF)
+ .append(") samples.getOrDefault(\"")
+ .append(MALCodegenHelper.escapeJava(expr.getMetricName()))
+ .append("\", ").append(SF).append(".EMPTY));\n");
+ chainCodegen.emitChainStatements(sb, expr.getMethodChain());
+ }
+
+ /**
+ * Generates statements for {@code (metric * 2).sum([...])}.
+ */
+ private void generateParenChainStatements(
+ final StringBuilder sb,
+ final MALExpressionModel.ParenChainExpr expr) {
+ generateExprStatements(sb, expr.getInner());
+ chainCodegen.emitChainStatements(sb, expr.getMethodChain());
+ }
+
+ /**
+ * Generates statements for top-level functions: {@code count(metric, ['tag'])},
+ * {@code topN(metric, 10, Order.DES)}.
+ */
+ private void generateFunctionCallStatements(
+ final StringBuilder sb,
+ final MALExpressionModel.FunctionCallExpr expr) {
+ final String fn = expr.getFunctionName();
+ final List<MALExpressionModel.Argument> args = expr.getArguments();
+
+ if (("count".equals(fn) || "topN".equals(fn)) && !args.isEmpty()) {
+ final MALExpressionModel.Argument firstArg = args.get(0);
+ if (firstArg instanceof MALExpressionModel.ExprArgument) {
+ generateExprStatements(
+ sb, ((MALExpressionModel.ExprArgument) firstArg).getExpr());
+ }
+ sb.append(" ").append(RUN_VAR).append(" = ")
+ .append(RUN_VAR).append('.').append(fn).append('(');
+ for (int i = 1; i < args.size(); i++) {
+ if (i > 1) {
+ sb.append(", ");
+ }
+ chainCodegen.generateArgument(sb, args.get(i));
+ }
+ sb.append(");\n");
+ } else {
+ sb.append(" ").append(RUN_VAR).append(" = ")
+ .append(fn).append('(');
+ for (int i = 0; i < args.size(); i++) {
+ if (i > 0) {
+ sb.append(", ");
+ }
+ chainCodegen.generateArgument(sb, args.get(i));
+ }
+ sb.append(");\n");
+ }
+ chainCodegen.emitChainStatements(sb, expr.getMethodChain());
+ }
+
+ /**
+ * Generates statements for binary arithmetic: {@code metric * 100},
+ * {@code metric1 + metric2}, {@code 2 / metric}.
+ *
+ * <p>Three cases:
+ * <ul>
+ * <li>N op SF: {@code sf = sf.plus(Long.valueOf(100L));} (swapped)</li>
+ * <li>SF op N: {@code sf = sf.multiply(Long.valueOf(100L));}</li>
+ * <li>SF op SF: uses temp variable {@code _t0} for left operand</li>
+ * </ul>
+ */
+ private void generateBinaryExprStatements(
+ final StringBuilder sb,
+ final MALExpressionModel.BinaryExpr expr) {
+ final MALExpressionModel.Expr left = expr.getLeft();
+ final MALExpressionModel.Expr right = expr.getRight();
+ final MALExpressionModel.ArithmeticOp op = expr.getOp();
+
+ final boolean leftIsNumber = isScalar(left);
+ final boolean rightIsNumber = isScalar(right);
+
+ if (leftIsNumber && !rightIsNumber) {
+ generateExprStatements(sb, right);
+ sb.append(" ").append(RUN_VAR).append(" = ");
+ switch (op) {
+ case ADD:
+ sb.append(RUN_VAR).append(".plus(");
+ generateScalarExprAsNumber(sb, left);
+ sb.append(')');
+ break;
+ case SUB:
+ sb.append(RUN_VAR).append(".minus(");
+ generateScalarExprAsNumber(sb, left);
+ sb.append(").negative()");
+ break;
+ case MUL:
+ sb.append(RUN_VAR).append(".multiply(");
+ generateScalarExprAsNumber(sb, left);
+ sb.append(')');
+ break;
+ case DIV:
+ sb.append(
+ "org.apache.skywalking.oap.meter.analyzer.v2.compiler.rt")
+ .append(".MalRuntimeHelper.divReverse(");
+ generateScalarExpr(sb, left);
+ sb.append(", ").append(RUN_VAR).append(")");
+ break;
+ default:
+ throw new IllegalArgumentException("Unsupported op: " + op);
+ }
+ sb.append(";\n");
+ } else if (!leftIsNumber && rightIsNumber) {
+ generateExprStatements(sb, left);
+ sb.append(" ").append(RUN_VAR).append(" = ")
+ .append(RUN_VAR).append(".")
+ .append(MALCodegenHelper.opMethodName(op)).append('(');
+ generateScalarExprAsNumber(sb, right);
+ sb.append(");\n");
+ } else {
+ generateExprStatements(sb, left);
+ final String temp = nextTemp();
+ sb.append(" ").append(SF).append(" ").append(temp)
+ .append(" = ").append(RUN_VAR).append(";\n");
+ generateExprStatements(sb, right);
+ sb.append(" ").append(RUN_VAR).append(" = ")
+ .append(temp).append(".")
+ .append(MALCodegenHelper.opMethodName(op))
+ .append("(").append(RUN_VAR).append(");\n");
+ }
+ }
+
+ // ==================== Inline expression codegen ====================
+
+ /**
+ * Generates an expression as a single inline Java expression (no statements).
+ * Used for sub-expressions in binary ops and function call arguments.
+ */
+ void generateExprInline(final StringBuilder sb,
+ final MALExpressionModel.Expr expr) {
+ if (expr instanceof MALExpressionModel.MetricExpr) {
+ generateMetricExprInline(sb, (MALExpressionModel.MetricExpr) expr);
+ } else if (expr instanceof MALExpressionModel.NumberExpr) {
+ final double val = ((MALExpressionModel.NumberExpr) expr).getValue();
+ sb.append(SF).append(".EMPTY.plus(");
+ MALCodegenHelper.emitNumberValueOf(sb, val);
+ sb.append(')');
+ } else if (expr instanceof MALExpressionModel.BinaryExpr) {
+ generateBinaryExprInline(
+ sb, (MALExpressionModel.BinaryExpr) expr);
+ } else if (expr instanceof MALExpressionModel.UnaryNegExpr) {
+ sb.append("(");
+ generateExprInline(
+ sb, ((MALExpressionModel.UnaryNegExpr) expr).getOperand());
+ sb.append(").negative()");
+ } else if (expr instanceof MALExpressionModel.FunctionCallExpr) {
+ generateFunctionCallExprInline(
+ sb, (MALExpressionModel.FunctionCallExpr) expr);
+ } else if (expr instanceof MALExpressionModel.ParenChainExpr) {
+ generateParenChainExprInline(
+ sb, (MALExpressionModel.ParenChainExpr) expr);
Review Comment:
generateExprInline() has no final else branch to throw on unknown Expr implementations. In contrast, the statement-based generator throws IllegalArgumentException for unknown types. Adding the same fail-fast behavior here would prevent silently emitting incomplete/invalid Java if new AST node types are introduced later or an unexpected node reaches inline generation.
```suggestion
sb, (MALExpressionModel.ParenChainExpr) expr);
} else {
throw new IllegalArgumentException(
"Unsupported inline expression type: " + (expr == null ? "null" : expr.getClass()));
```
--
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