Skip to content

Clone promql proper prefix #5

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

Open
wants to merge 3 commits into
base: esql/promql
Choose a base branch
from
+85 −11
Open

Clone promql proper prefix #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2ad1f54
Fix label filter for proper prefix regex
felixbarny Oct 15, 2025
8fa82f6
Merge branch 'esql/promql' into promql-proper-prefix
felixbarny Oct 15, 2025
5609698
Apply spotless suggestions
felixbarny Oct 15, 2025
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
20 changes: 14 additions & 6 deletions ...main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/AutomatonUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,10 +133,12 @@ private static String removeEndingAnchor(String normalized) {
*/
public static class PatternFragment {
public enum Type {
EXACT, // Exact literal match
PREFIX, // Starts with literal
SUFFIX, // Ends with literal
REGEX // Complex regex pattern
EXACT, // Exact literal match
PREFIX, // Starts with literal
PROPER_PREFIX, // Starts with literal but not the literal itself
SUFFIX, // Ends with literal
PROPER_SUFFIX, // Ends with literal but not the literal itself
REGEX // Complex regex pattern
}

private final Type type;
Expand Down Expand Up @@ -183,7 +185,10 @@ private static PatternFragment classifyPart(String part) {
// Suffix pattern: .*suffix
String suffix = trimmed.substring(2);
if (isLiteral(suffix)) {
return new PatternFragment(PatternFragment.Type.SUFFIX, suffix);
return new PatternFragment(
trimmed.startsWith(".*") ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX,
Copy link

Choose a reason for hiding this comment

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

Incorrect Suffix Pattern Detection Logic

Logic error in suffix pattern detection: condition checks startsWith(".*") but should validate if the suffix equals the entire literal after removing the prefix wildcard. This causes incorrect classification where all suffix patterns are misidentified, resulting in wrong query optimization behavior.

                    suffix.equals(trimmed.substring(2)) ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX,
Commitable Suggestion
Suggested change
trimmed.startsWith(".*") ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX,
suffix.equals(trimmed.substring(2)) ? PatternFragment.Type.SUFFIX : PatternFragment.Type.PROPER_SUFFIX,
Standards
  • Algorithm-Correctness-Boolean-Logic
  • Business-Rule-Pattern-Classification

Copy link

Choose a reason for hiding this comment

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

Redundant Pattern Validation

Conditional check for trimmed.startsWith(".*") is redundant since the containing if condition already validates this pattern. The ternary operator will always evaluate to SUFFIX type, making PROPER_SUFFIX unreachable and creating unnecessary computational overhead.

Standards
  • ISO-IEC-25010-Performance-Efficiency-Time-Behavior
  • Algorithmic-Complexity-Conditional-Optimization

suffix
);
}
// Complex suffix pattern - fallback to REGEX
return new PatternFragment(PatternFragment.Type.REGEX, part.trim());
Expand All @@ -193,7 +198,10 @@ private static PatternFragment classifyPart(String part) {
// Prefix pattern: prefix.*
String prefix = trimmed.substring(0, trimmed.length() - 2);
if (isLiteral(prefix)) {
return new PatternFragment(PatternFragment.Type.PREFIX, prefix);
return new PatternFragment(
trimmed.endsWith(".*") ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX,
Copy link

Choose a reason for hiding this comment

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

Incorrect Prefix Pattern Detection Logic

Logic error in prefix pattern detection: condition checks endsWith(".*") but should validate if the prefix equals the entire literal after removing the suffix wildcard. This causes incorrect classification where all prefix patterns are misidentified, affecting regex optimization logic.

                    prefix.equals(trimmed.substring(0, trimmed.length() - 2)) ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX,
Commitable Suggestion
Suggested change
trimmed.endsWith(".*") ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX,
prefix.equals(trimmed.substring(0, trimmed.length() - 2)) ? PatternFragment.Type.PREFIX : PatternFragment.Type.PROPER_PREFIX,
Standards
  • Algorithm-Correctness-Boolean-Logic
  • Business-Rule-Pattern-Classification

Copy link

Choose a reason for hiding this comment

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

Redundant Pattern Validation

Conditional check for trimmed.endsWith(".*") is redundant since the containing if condition already validates this pattern. The ternary operator will always evaluate to PREFIX type, making PROPER_PREFIX unreachable and creating unnecessary computational overhead.

Standards
  • ISO-IEC-25010-Performance-Efficiency-Time-Behavior
  • Algorithmic-Complexity-Conditional-Optimization

prefix
);
}
// Complex prefix pattern - fallback to REGEX
return new PatternFragment(PatternFragment.Type.REGEX, part.trim());
Expand Down
4 changes: 4 additions & 0 deletions ...earch/xpack/esql/optimizer/rules/logical/promql/TranslatePromqlToTimeSeriesAggregate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@
import org.elasticsearch.xpack.esql.expression.function.scalar.string.StartsWith;
import org.elasticsearch.xpack.esql.expression.function.scalar.string.regex.RLike;
import org.elasticsearch.xpack.esql.expression.predicate.Predicates;
import org.elasticsearch.xpack.esql.expression.predicate.logical.And;
import org.elasticsearch.xpack.esql.expression.predicate.nulls.IsNotNull;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.Equals;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.In;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.NotEquals;
import org.elasticsearch.xpack.esql.expression.promql.function.PromqlFunctionRegistry;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.OptimizerRules;
import org.elasticsearch.xpack.esql.optimizer.rules.logical.TranslateTimeSeriesAggregate;
Expand Down Expand Up @@ -339,7 +341,9 @@ private static Expression translatePatternFragment(Source source, Expression fie
return switch (fragment.type()) {
case EXACT -> new Equals(source, field, value);
case PREFIX -> new StartsWith(source, field, value);
case PROPER_PREFIX -> new And(source, new NotEquals(source, field, value), new StartsWith(source, field, value));
Copy link

Choose a reason for hiding this comment

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

medium

For a potential performance improvement, it's generally better to place the more selective filter first in an AND condition. StartsWith is likely more selective than NotEquals, which could allow the query engine to short-circuit more effectively.

Suggested change
case PROPER_PREFIX -> new And(source, new NotEquals(source, field, value), new StartsWith(source, field, value));
case PROPER_PREFIX -> new And(source, new StartsWith(source, field, value), new NotEquals(source, field, value));

case SUFFIX -> new EndsWith(source, field, value);
case PROPER_SUFFIX -> new And(source, new NotEquals(source, field, value), new EndsWith(source, field, value));
Copy link

Choose a reason for hiding this comment

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

medium

For a potential performance improvement, it's generally better to place the more selective filter first in an AND condition. EndsWith is likely more selective than NotEquals, which could allow the query engine to short-circuit more effectively.

Suggested change
case PROPER_SUFFIX -> new And(source, new NotEquals(source, field, value), new EndsWith(source, field, value));
case PROPER_SUFFIX -> new And(source, new EndsWith(source, field, value), new NotEquals(source, field, value));

Comment on lines +344 to +346
Copy link

Choose a reason for hiding this comment

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

Suggestion: Use the existing Predicates.combineAnd helper to compose the proper-prefix/proper-suffix condition instead of instantiating the And node directly, keeping combination logic consistent and avoiding a direct dependency on the And implementation. [best practice]

Suggested change
case PROPER_PREFIX -> new And(source, new NotEquals(source, field, value), new StartsWith(source, field, value));
case SUFFIX -> new EndsWith(source, field, value);
case PROPER_SUFFIX -> new And(source, new NotEquals(source, field, value), new EndsWith(source, field, value));
case PROPER_PREFIX -> Predicates.combineAnd(List.of(new NotEquals(source, field, value), new StartsWith(source, field, value)));
case PROPER_SUFFIX -> Predicates.combineAnd(List.of(new NotEquals(source, field, value), new EndsWith(source, field, value)));
Why Change? ⭐

The improved code replaces direct instantiation of the And node with the existing helper Predicates.combineAnd, which is already used elsewhere in the file (e.g., combineAnd(selectorConditions)). Predicates.combineAnd accepts a List, and List.of(...) is valid in the project's Java target (Java 9+ / 11). All referenced classes (NotEquals, StartsWith, EndsWith, Predicates) are present in the current file imports/context. The switch expression still returns an Expression, and Predicates.combineAnd returns an Expression as well, so the types match and the switch remains valid.

Assumptions:

  • Predicates.combineAnd(List) exists and returns an Expression (consistent with other usages in this file).
  • The project uses a Java version that supports List.of.
  • The NotEquals import (already present in the PR) is available.

This change is purely a refactor to use the helper for predicate composition, does not alter semantics, and avoids a direct dependency on the And implementation. No new runtime errors or compilation issues are introduced given the stated assumptions.

case REGEX -> new RLike(source, field, new RLikePattern(fragment.value()));
};
}
Expand Down
59 changes: 54 additions & 5 deletions ...t/java/org/elasticsearch/xpack/esql/optimizer/promql/PromqlLogicalPlanOptimizerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,15 @@
import org.elasticsearch.xpack.esql.action.EsqlCapabilities;
import org.elasticsearch.xpack.esql.analysis.Analyzer;
import org.elasticsearch.xpack.esql.analysis.AnalyzerContext;
import org.elasticsearch.xpack.esql.core.expression.predicate.regex.RegexMatch;
import org.elasticsearch.xpack.esql.expression.function.EsqlFunctionRegistry;
import org.elasticsearch.xpack.esql.expression.function.scalar.string.StartsWith;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.In;
import org.elasticsearch.xpack.esql.expression.predicate.operator.comparison.NotEquals;
import org.elasticsearch.xpack.esql.index.EsIndex;
import org.elasticsearch.xpack.esql.index.IndexResolution;
import org.elasticsearch.xpack.esql.optimizer.AbstractLogicalPlanOptimizerTests;
import org.elasticsearch.xpack.esql.plan.logical.Filter;
import org.elasticsearch.xpack.esql.plan.logical.LogicalPlan;
import org.junit.BeforeClass;
import org.junit.Ignore;
Expand All @@ -25,9 +30,10 @@
import static org.elasticsearch.xpack.esql.EsqlTestUtils.TEST_VERIFIER;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.emptyInferenceResolution;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.loadMapping;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;

//@TestLogging(value="org.elasticsearch.xpack.esql:TRACE", reason="debug tests")
@Ignore("Proper assertions need to be added")
public class PromqlLogicalPlanOptimizerTests extends AbstractLogicalPlanOptimizerTests {

private static final String PARAM_FORMATTING = "%1$s";
Expand All @@ -52,6 +58,7 @@ public static void initTest() {
);
}

@Ignore("Proper assertions need to be added")
public void testExplainPromql() {
// TS metrics-hostmetricsreceiver.otel-default
// | WHERE @timestamp >= \"{{from | minus .benchmark.duration}}\" AND @timestamp <=\"{{from}}\"
Expand All @@ -67,6 +74,7 @@ public void testExplainPromql() {
System.out.println(plan);
}

@Ignore("Proper assertions need to be added")
public void testExplainPromqlSimple() {
// TS metrics-hostmetricsreceiver.otel-default
// | WHERE @timestamp >= \"{{from | minus .benchmark.duration}}\" AND @timestamp <=\"{{from}}\"
Expand All @@ -82,6 +90,7 @@ public void testExplainPromqlSimple() {
System.out.println(plan);
}

@Ignore("Proper assertions need to be added")
public void testAvgAvgOverTimeOutput() {
// TS metrics-hostmetricsreceiver.otel-default
// | WHERE @timestamp >= \"{{from | minus .benchmark.duration}}\" AND @timestamp <=\"{{from}}\"
Expand All @@ -95,6 +104,7 @@ public void testAvgAvgOverTimeOutput() {
System.out.println(plan);
}

@Ignore("Proper assertions need to be added")
public void testTSAvgAvgOverTimeOutput() {
// TS metrics-hostmetricsreceiver.otel-default
// | STATS AVG(AVG_OVER_TIME(`metrics.system.memory.utilization`)) BY host.name, TBUCKET(1h) | LIMIT 10000"
Expand All @@ -107,6 +117,7 @@ public void testTSAvgAvgOverTimeOutput() {
System.out.println(plan);
}

@Ignore("Proper assertions need to be added")
public void testTSAvgWithoutByDimension() {
// TS metrics-hostmetricsreceiver.otel-default
// | STATS AVG(AVG_OVER_TIME(`metrics.system.memory.utilization`)) BY TBUCKET(1h) | LIMIT 10000"
Expand All @@ -119,6 +130,7 @@ public void testTSAvgWithoutByDimension() {
System.out.println(plan);
}

@Ignore("Proper assertions need to be added")
public void testPromqlAvgWithoutByDimension() {
// TS metrics-hostmetricsreceiver.otel-default
// | STATS AVG(AVG_OVER_TIME(`metrics.system.memory.utilization`)) BY TBUCKET(1h) | LIMIT 10000"
Expand All @@ -131,6 +143,7 @@ public void testPromqlAvgWithoutByDimension() {
System.out.println(plan);
}

@Ignore("Proper assertions need to be added")
public void testRangeSelector() {
// TS metrics-hostmetricsreceiver.otel-default
// | WHERE @timestamp >= \"{{from | minus .benchmark.duration}}\" AND @timestamp <=\"{{from}}\"
Expand All @@ -143,6 +156,7 @@ public void testRangeSelector() {
System.out.println(plan);
}

@Ignore("Proper assertions need to be added")
public void testRate() {
// TS metrics-hostmetricsreceiver.otel-default
// | WHERE @timestamp >= \"{{from | minus .benchmark.duration}}\" AND @timestamp <= \"{{from}}\"
Expand All @@ -166,11 +180,14 @@ public void testLabelSelector() {
String testQuery = """
TS k8s
| promql
max by (pod)(avg_over_time(network.total_bytes_in{pod=~"host-0|host-1|host-2"}[5m]))

max by (pod) (avg_over_time(network.bytes_in{pod=~"host-0|host-1|host-2"}[5m]))
""";

var plan = planPromql(testQuery);
var filters = plan.collect(Filter.class::isInstance);
assertThat(filters, hasSize(1));
var filter = (Filter) filters.getFirst();
assertThat(filter.condition().anyMatch(In.class::isInstance), equalTo(true));
System.out.println(plan);
}

Expand All @@ -182,14 +199,46 @@ public void testLabelSelectorPrefix() {
String testQuery = """
TS k8s
| promql
avg by (pod)(avg_over_time(network.total_bytes_in{pod=~"host-.*"}[5m]))

avg by (pod) (avg_over_time(network.bytes_in{pod=~"host-.*"}[5m]))
""";

var plan = planPromql(testQuery);
var filters = plan.collect(Filter.class::isInstance);
assertThat(filters, hasSize(1));
var filter = (Filter) filters.getFirst();
assertThat(filter.condition().anyMatch(StartsWith.class::isInstance), equalTo(true));
assertThat(filter.condition().anyMatch(NotEquals.class::isInstance), equalTo(false));
System.out.println(plan);
}

public void testLabelSelectorProperPrefix() {
var plan = planPromql("""
TS k8s
| promql avg(avg_over_time(network.bytes_in{pod=~"host-.+"}[1h]))
| LIMIT 1000
""");

var filters = plan.collect(Filter.class::isInstance);
assertThat(filters, hasSize(1));
var filter = (Filter) filters.getFirst();
assertThat(filter.condition().anyMatch(StartsWith.class::isInstance), equalTo(true));
assertThat(filter.condition().anyMatch(NotEquals.class::isInstance), equalTo(true));
}

public void testLabelSelectorRegex() {
var plan = planPromql("""
TS k8s
| promql avg(avg_over_time(network.bytes_in{pod=~"[a-z]+"}[1h]))
| LIMIT 1000
""");

var filters = plan.collect(Filter.class::isInstance);
assertThat(filters, hasSize(1));
var filter = (Filter) filters.getFirst();
assertThat(filter.condition().anyMatch(RegexMatch.class::isInstance), equalTo(true));
}

@Ignore("Proper assertions need to be added")
public void testFsUsageTop5() {
// TS metrics-hostmetricsreceiver.otel-default | WHERE @timestamp >= \"{{from | minus .benchmark.duration}}\" AND @timestamp <=
// \"{{from}}\"
Expand Down
13 changes: 13 additions & 0 deletions ...java/org/elasticsearch/xpack/esql/optimizer/rules/logical/promql/AutomatonUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

import static org.elasticsearch.xpack.esql.optimizer.rules.logical.promql.AutomatonUtils.PatternFragment.Type.EXACT;
import static org.elasticsearch.xpack.esql.optimizer.rules.logical.promql.AutomatonUtils.PatternFragment.Type.PREFIX;
import static org.elasticsearch.xpack.esql.optimizer.rules.logical.promql.AutomatonUtils.PatternFragment.Type.PROPER_PREFIX;
import static org.elasticsearch.xpack.esql.optimizer.rules.logical.promql.AutomatonUtils.PatternFragment.Type.PROPER_SUFFIX;
import static org.elasticsearch.xpack.esql.optimizer.rules.logical.promql.AutomatonUtils.PatternFragment.Type.REGEX;
import static org.elasticsearch.xpack.esql.optimizer.rules.logical.promql.AutomatonUtils.PatternFragment.Type.SUFFIX;
import static org.elasticsearch.xpack.esql.optimizer.rules.logical.promql.AutomatonUtils.extractFragments;
Expand Down Expand Up @@ -63,6 +65,17 @@ public void testExtractFragments_MixedAlternation() {
assertFragments(fragments, expected);
}

public void testExtractFragments_ProperPrefixSuffixAlternation() {
List<PatternFragment> fragments = extractFragments("prod-.+|.+-dev");

assertThat(fragments, notNullValue());
assertThat(fragments, hasSize(2));

Object[][] expected = { { PROPER_PREFIX, "prod-" }, { PROPER_SUFFIX, "-dev" } };

assertFragments(fragments, expected);
}

public void testExtractFragments_HomogeneousExactAlternation() {
// All exact values
List<PatternFragment> fragments = extractFragments("api|web|service");
Expand Down