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));