Skip to content

Fix UnresolvedException on PromQL by(step) grouping #145307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
felixbarny merged 1 commit into from
Mar 31, 2026
Merged

Fix UnresolvedException on PromQL by(step) grouping #145307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions ...src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/AcrossSeriesAggregate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
* <p>
Expand Down Expand Up @@ -115,7 +113,7 @@ public List<Attribute> 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();
Copy link
Copy Markdown
Contributor

@alex-spies alex-spies Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felixbarny , I know this is already merged, but this doesn't look right. Unresolved attributes must not be present when output is called.

The javadoc for the output method states:

     * Must be called only on resolved plans, otherwise may throw an exception or return wrong results.

Copy link
Copy Markdown
Member Author

@felixbarny felixbarny Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wasn't the primary fix but a defensive check. Maybe too defensive in this case...
Maybe better to fail early or add something like an assert resolved() here.

Copy link
Copy Markdown
Member Author

@felixbarny felixbarny Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a PR to add the assert: #145525

}

@Override
Expand Down
17 changes: 16 additions & 1 deletion ...in/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/PromqlCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 [{}]",
Copy link
Copy Markdown
Member

@kkrik-es kkrik-es Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine as a stopgap, but I wonder if we can differentiate between the two. Maybe we can implicitly add an alias to the label, to avoid the conflict?

Copy link
Copy Markdown
Member Author

@felixbarny felixbarny Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can implicitly add an alias to the label, to avoid the conflict?

What do you have in mind there? That'll still change the name of the output column, so I'm not sure if that'll work.

stepColumnName(),
stepColumnName(),
agg.sourceText()
)
);
}
}
}
case PromqlFunctionCall functionCall -> {
}
Expand Down
13 changes: 13 additions & 0 deletions ...l/src/test/java/org/elasticsearch/xpack/esql/optimizer/promql/PromqlEsqlCommandTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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));
Copy link
Copy Markdown
Member

@kkrik-es kkrik-es Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this converted to 4xx or 5xx? Can't recall where the mapping is defined.

Copy link
Copy Markdown
Member Author

@felixbarny felixbarny Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation errors are always client errors (4xx).

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
Expand Down
2 changes: 1 addition & 1 deletion ...metheus/src/main/java/org/elasticsearch/xpack/prometheus/rest/PromqlQueryPlanBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Loading