Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
1c151f5
Add support for selectors in ES|QL
jbaiera Jan 22, 2025
9e70be8
Add selector separator to metrics command.
jbaiera Jan 22, 2025
3fb8d9d
Add parsing tests
jbaiera Jan 22, 2025
8856a0d
Add integration test for parsing failure store syntax
jbaiera Jan 24, 2025
4be6984
Merge branch 'main' into failure-store-esql-support
jbaiera Jan 24, 2025
4b92237
[CI] Auto commit changes from spotless
Jan 24, 2025
e2297ed
PR Feedback
jbaiera Jan 27, 2025
278800c
Merge branch 'main' into failure-store-esql-support
jbaiera Jan 27, 2025
87d0897
Rethrow as ParsingException
jbaiera Jan 28, 2025
9fd861e
Add feature flagged capability and some more testing
jbaiera Jan 28, 2025
28e5825
Add invalid pattern case
jbaiera Jan 28, 2025
418e12a
Add join and lookup selectors
jbaiera Jan 29, 2025
e39ca3a
Add selectors to identifier generator
jbaiera Jan 29, 2025
738eef0
Regen
jbaiera Jan 29, 2025
12fdb9b
Add invalid join pattern test assertion for selectors
jbaiera Jan 29, 2025
9b635f3
Simplify implementation to remove join and lookup support for selectors.
jbaiera Feb 10, 2025
aea1d89
Merge branch 'main' into failure-store-esql-support
jbaiera Feb 11, 2025
60a8011
Merge branch 'main' into failure-store-esql-support
jbaiera Feb 11, 2025
d69ea04
Unify selector syntax with cluster syntax
jbaiera Feb 11, 2025
208cea6
Disallow cluster strings when used with selector strings
jbaiera Feb 20, 2025
8df5cc2
Regen
jbaiera Feb 20, 2025
b3edb8d
Make cluster name and selector exclusive of one another.
jbaiera Feb 21, 2025
16831d7
Merge branch 'main' into failure-store-esql-support
jbaiera Feb 21, 2025
c11fbc6
Update tests for removal of ::*
jbaiera Feb 21, 2025
eca0b0c
Merge branch 'main' into failure-store-esql-support
jbaiera Mar 11, 2025
c88f1f8
Re-add metrics lexer code and regen
jbaiera Mar 11, 2025
7d582ba
[CI] Auto commit changes from spotless
Mar 11, 2025
577e15d
re-add missing lexer entry for selector syntax
jbaiera Mar 14, 2025
c0bc7d8
Merge branch 'main' into failure-store-esql-support
jbaiera Mar 14, 2025
f13b674
Fix tests
jbaiera Mar 17, 2025
c101724
Merge branch 'main' into failure-store-esql-support
jbaiera Mar 17, 2025
2373537
regen
jbaiera Mar 17, 2025
67a7ccd
Fix another test
jbaiera Mar 17, 2025
c4b0a93
Rework error message from IdentifierBuilder
jbaiera Mar 20, 2025
6d5b049
Expand test cases for join query. Update join query to use correct sy…
jbaiera Mar 20, 2025
364bbd4
[CI] Auto commit changes from spotless
Mar 20, 2025
7f8680d
Clean up test debugging
jbaiera Mar 20, 2025
80088a0
Merge branch 'main' into failure-store-esql-support
jbaiera Mar 21, 2025
29ed978
regen
jbaiera Mar 21, 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
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import java.util.function.Function;
import java.util.function.LongSupplier;
import java.util.function.Predicate;
import java.util.function.Supplier;

/**
* This class main focus is to resolve multi-syntax target expressions to resources or concrete indices. This resolution is influenced
Expand Down Expand Up @@ -2148,23 +2149,7 @@ private static <V> V splitSelectorExpression(String expression, BiFunction<Strin
int lastDoubleColon = expression.lastIndexOf(SELECTOR_SEPARATOR);
if (lastDoubleColon >= 0) {
String suffix = expression.substring(lastDoubleColon + SELECTOR_SEPARATOR.length());
IndexComponentSelector selector = IndexComponentSelector.getByKey(suffix);
if (selector == null) {
// Do some work to surface a helpful error message for likely errors
if (Regex.isSimpleMatchPattern(suffix)) {
throw new InvalidIndexNameException(
expression,
"Invalid usage of :: separator, ["
+ suffix
+ "] contains a wildcard, but only the match all wildcard [*] is supported in a selector"
);
} else {
throw new InvalidIndexNameException(
expression,
"Invalid usage of :: separator, [" + suffix + "] is not a recognized selector"
);
}
}
doValidateSelectorString(() -> expression, suffix);
String expressionBase = expression.substring(0, lastDoubleColon);
ensureNoMoreSelectorSeparators(expressionBase, expression);
return bindFunction.apply(expressionBase, suffix);
Expand All @@ -2173,6 +2158,30 @@ private static <V> V splitSelectorExpression(String expression, BiFunction<Strin
return bindFunction.apply(expression, null);
}

public static void validateIndexSelectorString(String indexName, String suffix) {
doValidateSelectorString(() -> indexName + SELECTOR_SEPARATOR + suffix, suffix);
}

private static void doValidateSelectorString(Supplier<String> expression, String suffix) {
IndexComponentSelector selector = IndexComponentSelector.getByKey(suffix);
if (selector == null) {
// Do some work to surface a helpful error message for likely errors
if (Regex.isSimpleMatchPattern(suffix)) {
throw new InvalidIndexNameException(
expression.get(),
"Invalid usage of :: separator, ["
+ suffix
+ "] contains a wildcard, but only the match all wildcard [*] is supported in a selector"
);
} else {
throw new InvalidIndexNameException(
expression.get(),
"Invalid usage of :: separator, [" + suffix + "] is not a recognized selector"
);
}
}
}

/**
* Checks the selectors that have been returned from splitting an expression and throws an exception if any were present.
* @param expression Original expression
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugin/esql/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ dependencies {
testImplementation project(path: ':modules:analysis-common')
testImplementation project(path: ':modules:ingest-common')
testImplementation project(path: ':modules:legacy-geo')
testImplementation project(path: ':modules:data-streams')
testImplementation project(path: ':modules:mapper-extras')
testImplementation project(xpackModule('esql:compute:test'))
testImplementation('net.nextencia:rrdiagram:0.9.4')
testImplementation('org.webjars.npm:fontsource__roboto-mono:4.5.7')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,43 @@
package org.elasticsearch.xpack.esql.action;

import org.elasticsearch.Build;
import org.elasticsearch.action.DocWriteRequest;
import org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequest;
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest;
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequestBuilder;
import org.elasticsearch.action.admin.indices.template.delete.TransportDeleteComposableIndexTemplateAction;
import org.elasticsearch.action.admin.indices.template.put.TransportPutComposableIndexTemplateAction;
import org.elasticsearch.action.bulk.BulkRequestBuilder;
import org.elasticsearch.action.bulk.BulkResponse;
import org.elasticsearch.action.datastreams.DeleteDataStreamAction;
import org.elasticsearch.action.index.IndexRequest;
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.support.WriteRequest;
import org.elasticsearch.client.internal.ClusterAdminClient;
import org.elasticsearch.cluster.metadata.ComposableIndexTemplate;
import org.elasticsearch.cluster.metadata.DataStream;
import org.elasticsearch.cluster.metadata.DataStreamFailureStore;
import org.elasticsearch.cluster.metadata.DataStreamOptions;
import org.elasticsearch.cluster.metadata.IndexMetadata;
import org.elasticsearch.cluster.metadata.ResettableValue;
import org.elasticsearch.cluster.metadata.Template;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.common.collect.Iterators;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.datastreams.DataStreamsPlugin;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexService;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.mapper.DateFieldMapper;
import org.elasticsearch.index.mapper.extras.MapperExtrasPlugin;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.index.query.RangeQueryBuilder;
import org.elasticsearch.index.shard.IndexShard;
import org.elasticsearch.indices.IndicesService;
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.ListMatcher;
import org.elasticsearch.xcontent.XContentBuilder;
Expand All @@ -38,11 +55,13 @@
import org.elasticsearch.xpack.esql.parser.ParsingException;
import org.elasticsearch.xpack.esql.plugin.EsqlPlugin;
import org.elasticsearch.xpack.esql.plugin.QueryPragmas;
import org.junit.Assume;
import org.junit.Before;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
Expand All @@ -60,6 +79,7 @@
import java.util.concurrent.atomic.AtomicLong;
import java.util.stream.IntStream;
import java.util.stream.LongStream;
import java.util.stream.Stream;

import static java.util.Comparator.comparing;
import static java.util.Comparator.naturalOrder;
Expand Down Expand Up @@ -100,6 +120,11 @@ protected Settings nodeSettings(int nodeOrdinal, Settings otherSettings) {
.build();
}

@Override
protected Collection<Class<? extends Plugin>> nodePlugins() {
return Stream.concat(super.nodePlugins().stream(), Stream.of(DataStreamsPlugin.class, MapperExtrasPlugin.class)).toList();
}

public void testProjectConstant() {
try (EsqlQueryResponse results = run("from test | eval x = 1 | keep x")) {
assertThat(results.columns(), equalTo(List.of(new ColumnInfoImpl("x", "integer"))));
Expand Down Expand Up @@ -970,6 +995,152 @@ public void testIndexPatterns() throws Exception {
}
}

public void testDataStreamPatterns() throws Exception {
Assume.assumeTrue(DataStream.isFailureStoreFeatureFlagEnabled());

Map<String, Long> testCases = new HashMap<>();
testCases.put("test_ds_patterns*", 15L);
testCases.put("test_ds_patterns*::data", 15L);
testCases.put("test_ds_patterns*::failures", 9L);
testCases.put("test_ds_patterns*::*", 24L);

testCases.put("test_ds_patterns_1,test_ds_patterns_2", 10L);
testCases.put("test_ds_patterns_1::data,test_ds_patterns_2::data", 10L);
testCases.put("test_ds_patterns_1::failures,test_ds_patterns_2::failures", 6L);
testCases.put("test_ds_patterns_1::*,test_ds_patterns_2::*", 16L);

testCases.put("test_ds_patterns_1*,test_ds_patterns_2*", 10L);
testCases.put("test_ds_patterns_1*::data,test_ds_patterns_2*::data", 10L);
testCases.put("test_ds_patterns_1*::failures,test_ds_patterns_2*::failures", 6L);
testCases.put("test_ds_patterns_1*::*,test_ds_patterns_2*::*", 16L);

testCases.put("*", 15L);
testCases.put("*::data", 15L);
testCases.put("*::failures", 9L);
testCases.put("*::*", 24L);

testCases.put("test_ds_patterns_2", 5L);
testCases.put("test_ds_patterns_2::data", 5L);
testCases.put("test_ds_patterns_2::failures", 3L);
testCases.put("test_ds_patterns_2::*", 8L);

boolean deleteTemplate = false;
List<String> deleteDataStreams = new ArrayList<>();
try {
assertAcked(
client().execute(
TransportPutComposableIndexTemplateAction.TYPE,
new TransportPutComposableIndexTemplateAction.Request("test_ds_template").indexTemplate(
ComposableIndexTemplate.builder()
.indexPatterns(List.of("test_ds_patterns_*"))
.dataStreamTemplate(new ComposableIndexTemplate.DataStreamTemplate())
.template(
Template.builder()
.mappings(new CompressedXContent("""
{
"dynamic": false,
"properties": {
"@timestamp": {
"type": "date"
},
"count": {
"type": "long"
}
}
}"""))
.dataStreamOptions(
ResettableValue.create(
new DataStreamOptions.Template(
ResettableValue.create(new DataStreamFailureStore.Template(ResettableValue.create(true)))
)
)
)
.build()
)
.build()
)
)
);
deleteTemplate = true;

String[] dsNames = { "test_ds_patterns_1", "test_ds_patterns_2", "test_ds_patterns_3" };
String time = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.formatMillis(System.currentTimeMillis());
int i = 0;
for (String dsName : dsNames) {
BulkResponse bulkItemResponses = client().prepareBulk()
.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE)
.add(
new IndexRequest(dsName).opType(DocWriteRequest.OpType.CREATE)
.id("1")
.source("@timestamp", time, "count", ++i * 1000)
)
.add(
new IndexRequest(dsName).opType(DocWriteRequest.OpType.CREATE)
.id("2")
.source("@timestamp", time, "count", ++i * 1000)
)
.add(
new IndexRequest(dsName).opType(DocWriteRequest.OpType.CREATE)
.id("3")
.source("@timestamp", time, "count", ++i * 1000)
)
.add(
new IndexRequest(dsName).opType(DocWriteRequest.OpType.CREATE)
.id("4")
.source("@timestamp", time, "count", ++i * 1000)
)
.add(
new IndexRequest(dsName).opType(DocWriteRequest.OpType.CREATE)
.id("5")
.source("@timestamp", time, "count", ++i * 1000)
)
.add(
new IndexRequest(dsName).opType(DocWriteRequest.OpType.CREATE)
.id("6")
.source("@timestamp", time, "count", "garbage")
)
.add(
new IndexRequest(dsName).opType(DocWriteRequest.OpType.CREATE)
.id("7")
.source("@timestamp", time, "count", "garbage")
)
.add(
new IndexRequest(dsName).opType(DocWriteRequest.OpType.CREATE)
.id("8")
.source("@timestamp", time, "count", "garbage")
)
.get();
assertThat(bulkItemResponses.hasFailures(), is(false));
ensureYellow(dsName);
deleteDataStreams.add(dsName);
}

for (Map.Entry<String, Long> testCase : testCases.entrySet()) {
try (var results = run("from " + testCase.getKey() + " | stats count(@timestamp)")) {
assertEquals(testCase.getKey(), 1, getValuesList(results).size());
assertEquals(testCase.getKey(), testCase.getValue(), getValuesList(results).get(0).get(0));
}
}
} finally {
if (deleteDataStreams.isEmpty() == false) {
assertAcked(
client().execute(
DeleteDataStreamAction.INSTANCE,
new DeleteDataStreamAction.Request(new TimeValue(30, TimeUnit.SECONDS), deleteDataStreams.toArray(String[]::new))
)
);
}
if (deleteTemplate) {
assertAcked(
client().execute(
TransportDeleteComposableIndexTemplateAction.TYPE,
new TransportDeleteComposableIndexTemplateAction.Request("test_ds_template")
)
);
}
}
}

public void testOverlappingIndexPatterns() throws Exception {
String[] indexNames = { "test_overlapping_index_patterns_1", "test_overlapping_index_patterns_2" };

Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,7 @@ FROM_PIPE : PIPE -> type(PIPE), popMode;
FROM_OPENING_BRACKET : OPENING_BRACKET -> type(OPENING_BRACKET);
FROM_CLOSING_BRACKET : CLOSING_BRACKET -> type(CLOSING_BRACKET);
FROM_COLON : COLON -> type(COLON);
FROM_SELECTOR : {this.isDevVersion()}? CAST_OP -> type(CAST_OP);
FROM_COMMA : COMMA -> type(COMMA);
FROM_ASSIGN : ASSIGN -> type(ASSIGN);
METADATA : 'metadata';
Expand Down Expand Up @@ -608,6 +609,10 @@ CLOSING_METRICS_COLON
: COLON -> type(COLON), popMode, pushMode(METRICS_MODE)
;

CLOSING_METRICS_SELECTOR
: {this.isDevVersion()}? CAST_OP -> type(CAST_OP), popMode, pushMode(METRICS_MODE)
;

CLOSING_METRICS_COMMA
: COMMA -> type(COMMA), popMode, pushMode(METRICS_MODE)
;
Expand Down
5 changes: 5 additions & 0 deletions x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4
Original file line number Diff line number Diff line change
Expand Up @@ -139,12 +139,17 @@ fromCommand

indexPattern
: (clusterString COLON)? indexString
| {this.isDevVersion()}? (clusterString COLON)? indexString (CAST_OP selectorString)?
Copy link
Member

Choose a reason for hiding this comment

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

The indexPattern is used in a few other places like joinCommand and metricsCommand etc. Do we expect the selector work in the other commands besides fromCommand? If we don't want to support it under the other command, we can put the restrictions in LogicalPlanBuilder, and it will be great to have negative/positive tests to cover the them.

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to say that this should work in the joinCommand, but I don't know if there are any functional restrictions on that command that might bar the pattern's usage. I'm not sure I know what the metricsCommand actually is and what the difference is between it and the fromCommand.

Copy link
Member

Choose a reason for hiding this comment

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

Both joinCommand and metricsCommand are under snapshot today.

Here are something that I can think of that might affect ::, that we need to validate, the best way to validate them is to test them.

  • * is not supported on the right hand side of the join, however it is a valid :: option
var rightPattern = visitIndexPattern(List.of(target.index));
        if (rightPattern.contains(WILDCARD)) {
            throw new ParsingException(source(target), "invalid index pattern [{}], * is not allowed in LOOKUP JOIN", rightPattern);
        }
  • metrics is mainly for tsdb, I'm not quite sure if :: is relevant to tsdb honestly, if it is not relevant we can error out early in parsing, otherwise we may also want to have tests to show how it works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is the reason that wildcards are banned from the join command because they resolve to multiple indices? I can say for sure that a selector of ::* has a good chance of resolving to multiple indices. That said, putting any data stream name in the right hand side of the join operation will likely resolve to multiple backing indices, much like an alias pointing to multiple indices would. How do we handle aliases and data streams on the right hand side of the join today?

As for metrics and TSDB - the failure store doesn't surface any time series features like a time series data stream might. A time series data stream can have a failure store associated with it, but its failure store doesn't exhibit any of the TSDB functionality. If the expectation is that the metrics command is only ever used with TSDB, I don't know if it makes sense to allow selectors in it for the time being, since the only valid selectors are likely ::data in that case.

;

clusterString
: UNQUOTED_SOURCE
;

selectorString
: UNQUOTED_SOURCE
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we allow quoting selector?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried to simply follow how the definition for the clusterString are laid out. I don't have too much of an opinion on quoting the selector or not. The selector contents can only ever be data, failures, or *. I don't know what quoting them separately from the rest of the index pattern might buy us.

;

indexString
: UNQUOTED_SOURCE
| QUOTED_STRING
Expand Down

Large diffs are not rendered by default.

Loading