From 99f169af79b9ddd9a682b70152fe09b9e3a8f4f0 Mon Sep 17 00:00:00 2001 From: Felix Barnsteiner Date: Tue, 31 Mar 2026 12:32:17 +0200 Subject: [PATCH] Fix UnresolvedException on PromQL by(step) grouping Guard AcrossSeriesAggregate.output() against calling dataType() on unresolved attributes, which threw UnresolvedException during optimization. Reject PromQL by(step) groupings in postAnalysisVerification because the label name "step" collides with the built-in step time bucket output column. A future improvement could add an option to rename the built-in step column to support this. Also make STEP_COLUMN_NAME private and use stepColumnName() accessor throughout. --- .../logical/promql/AcrossSeriesAggregate.java | 4 +--- .../esql/plan/logical/promql/PromqlCommand.java | 17 ++++++++++++++++- .../promql/PromqlEsqlCommandTests.java | 13 +++++++++++++ .../prometheus/rest/PromqlQueryPlanBuilder.java | 2 +- 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/AcrossSeriesAggregate.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/AcrossSeriesAggregate.java index 203654aa5859f..f08f0d12ef6cd 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/AcrossSeriesAggregate.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/AcrossSeriesAggregate.java @@ -20,8 +20,6 @@ import java.util.List; import java.util.Objects; -import static java.util.function.Predicate.not; - /** * Represents a PromQL aggregate function call that operates across multiple time series. *

@@ -115,7 +113,7 @@ public List output() { if (grouping == Grouping.WITHOUT) { return List.of(timeseriesAttribute); } - return groupings.stream().filter(not(a -> a.dataType() == DataType.NULL)).toList(); + return groupings.stream().filter(a -> a.resolved() == false || a.dataType() != DataType.NULL).toList(); } @Override diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/PromqlCommand.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/PromqlCommand.java index a183d9a779bec..81bdd67b21748 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/PromqlCommand.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/PromqlCommand.java @@ -55,7 +55,7 @@ public class PromqlCommand extends UnaryPlan /** * The name of the column containing the step value (aka time bucket) in range queries. */ - public static final String STEP_COLUMN_NAME = "step"; + private static final String STEP_COLUMN_NAME = "step"; private final LogicalPlan promqlPlan; private final Literal start; @@ -407,6 +407,21 @@ public void postAnalysisVerification(Failures failures) { if (agg.grouping() == AcrossSeriesAggregate.Grouping.WITHOUT && usesWithoutGrouping(agg.child())) { failures.add(fail(agg, "nested WITHOUT over WITHOUT is not supported at this time [{}]", agg.sourceText())); } + // Reject labels whose name collides with the built-in step column. + // If this proves too restrictive, we could add an option to rename the built-in step column. + for (Attribute grouping : agg.groupings()) { + if (stepColumnName().equals(grouping.name())) { + failures.add( + fail( + agg, + "label [{}] collides with the built-in [{}] output column [{}]", + stepColumnName(), + stepColumnName(), + agg.sourceText() + ) + ); + } + } } case PromqlFunctionCall functionCall -> { } diff --git a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/promql/PromqlEsqlCommandTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/promql/PromqlEsqlCommandTests.java index 50a31d5031188..eb0dc58f3bd31 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/promql/PromqlEsqlCommandTests.java +++ b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/optimizer/promql/PromqlEsqlCommandTests.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.esql.optimizer.promql; +import org.elasticsearch.xpack.esql.VerificationException; import org.elasticsearch.xpack.esql.core.expression.Alias; import org.elasticsearch.xpack.esql.core.expression.Attribute; import org.elasticsearch.xpack.esql.core.expression.Expressions; @@ -83,6 +84,18 @@ public void testNonExistentFieldsOptimizesToEmptyPlan() { }); } + public void testGroupByStepCollision() { + // "step" as a BY label collides with the built-in step output column. + // If this proves too restrictive, we could add an option to rename the built-in step column. + for (String query : List.of( + "PROMQL index=k8s step=1m result=(sum by (step) (network.eth0.rx))", + "PROMQL index=k8s step=1m result=(sum by (step, pod) (network.eth0.rx))" + )) { + var e = expectThrows(VerificationException.class, () -> planPromql(query)); + assertThat(e.getMessage(), containsString("label [step] collides with the built-in [step] output column")); + } + } + public void testGroupByNonExistentLabel() { var plan = planPromql("PROMQL index=k8s step=1m result=(sum by (non_existent_label) (network.eth0.rx))"); // equivalent to avg(network.eth0.rx) since the label does not exist diff --git a/x-pack/plugin/prometheus/src/main/java/org/elasticsearch/xpack/prometheus/rest/PromqlQueryPlanBuilder.java b/x-pack/plugin/prometheus/src/main/java/org/elasticsearch/xpack/prometheus/rest/PromqlQueryPlanBuilder.java index 89c02ff87035c..b154cf28f7be1 100644 --- a/x-pack/plugin/prometheus/src/main/java/org/elasticsearch/xpack/prometheus/rest/PromqlQueryPlanBuilder.java +++ b/x-pack/plugin/prometheus/src/main/java/org/elasticsearch/xpack/prometheus/rest/PromqlQueryPlanBuilder.java @@ -77,7 +77,7 @@ static EsqlStatement buildStatement(String query, String index, String startStr, ); // TO_LONG converts the step datetime to epoch millis, avoiding the need to parse a date string in the response listener. - Alias stepAlias = new Alias(Source.EMPTY, PromqlCommand.STEP_COLUMN_NAME, new ToLong(Source.EMPTY, promqlCommand.stepAttribute())); + Alias stepAlias = new Alias(Source.EMPTY, promqlCommand.stepColumnName(), new ToLong(Source.EMPTY, promqlCommand.stepAttribute())); // Eval's mergeOutputAttributes drops step(datetime) and appends step_alias(long) at the end, // producing [value, ...dimensions, step(long)] — the order the response listener expects. Eval eval = new Eval(Source.EMPTY, promqlCommand, List.of(stepAlias));