Skip to content

Commit 94463ec

Browse files
jbaierajbaiera
committed
Simplify selector processing
1 parent e9daf21 commit 94463ec

File tree

6 files changed

+129
-41
lines changed

6 files changed

+129
-41
lines changed

‎x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/optimizer/OptimizerTests.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -753,7 +753,7 @@ private static Attribute tiebreaker() {
753753
}
754754

755755
private static LogicalPlan rel() {
756-
return new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, "catalog", "index", null), "", false);
756+
return new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, "catalog", "index"), "", false);
757757
}
758758

759759
private static KeyedFilter keyedFilter(LogicalPlan child) {

‎x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/plan/TableIdentifier.java‎

Lines changed: 11 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010

1111
import java.util.Objects;
1212

13-
import static org.elasticsearch.cluster.metadata.IndexNameExpressionResolver.SelectorResolver.SELECTOR_SEPARATOR;
1413
import static org.elasticsearch.transport.RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR;
1514

1615
public class TableIdentifier {
@@ -19,13 +18,11 @@ public class TableIdentifier {
1918

2019
private final String cluster;
2120
private final String index;
22-
private final String selector;
2321

24-
public TableIdentifier(Source source, String catalog, String index, String selector) {
22+
public TableIdentifier(Source source, String catalog, String index) {
2523
this.source = source;
2624
this.cluster = catalog;
2725
this.index = index;
28-
this.selector = selector;
2926
}
3027

3128
public String cluster() {
@@ -36,13 +33,9 @@ public String index() {
3633
return index;
3734
}
3835

39-
public String selector() {
40-
return selector;
41-
}
42-
4336
@Override
4437
public int hashCode() {
45-
return Objects.hash(cluster, index, selector);
38+
return Objects.hash(cluster, index);
4639
}
4740

4841
@Override
@@ -56,32 +49,25 @@ public boolean equals(Object obj) {
5649
}
5750

5851
TableIdentifier other = (TableIdentifier) obj;
59-
return Objects.equals(index, other.index) && Objects.equals(cluster, other.cluster) && Objects.equals(selector, other.selector);
52+
return Objects.equals(index, other.index) && Objects.equals(cluster, other.cluster);
6053
}
6154

6255
public Source source() {
6356
return source;
6457
}
6558

6659
public String qualifiedIndex() {
67-
if (cluster == null && selector == null) {
68-
return index;
69-
}
70-
StringBuilder qualifiedIndex = new StringBuilder();
71-
if (cluster != null) {
72-
qualifiedIndex.append(cluster);
73-
qualifiedIndex.append(REMOTE_CLUSTER_INDEX_SEPARATOR);
74-
}
75-
qualifiedIndex.append(index);
76-
if (selector != null) {
77-
qualifiedIndex.append(SELECTOR_SEPARATOR);
78-
qualifiedIndex.append(selector);
79-
}
80-
return qualifiedIndex.toString();
60+
return cluster != null ? cluster + REMOTE_CLUSTER_INDEX_SEPARATOR + index : index;
8161
}
8262

8363
@Override
8464
public String toString() {
85-
return qualifiedIndex();
65+
StringBuilder builder = new StringBuilder();
66+
if (cluster != null) {
67+
builder.append(cluster);
68+
builder.append(REMOTE_CLUSTER_INDEX_SEPARATOR);
69+
}
70+
builder.append(index);
71+
return builder.toString();
8672
}
8773
}

‎x-pack/plugin/ql/src/test/java/org/elasticsearch/xpack/ql/analyzer/PreAnalyzerTests.java‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,40 +25,37 @@ public class PreAnalyzerTests extends ESTestCase {
2525
private PreAnalyzer preAnalyzer = new PreAnalyzer();
2626

2727
public void testBasicIndex() {
28-
LogicalPlan plan = new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, null, "index", null), null, false);
28+
LogicalPlan plan = new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, null, "index"), null, false);
2929
PreAnalysis result = preAnalyzer.preAnalyze(plan);
3030
assertThat(plan.preAnalyzed(), is(true));
3131
assertThat(result.indices, hasSize(1));
3232
assertThat(result.indices.get(0).id().cluster(), nullValue());
3333
assertThat(result.indices.get(0).id().index(), is("index"));
34-
assertThat(result.indices.get(0).id().selector(), nullValue());
3534
}
3635

3736
public void testBasicIndexWithCatalog() {
38-
LogicalPlan plan = new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, "elastic", "index", null), null, false);
37+
LogicalPlan plan = new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, "elastic", "index"), null, false);
3938
PreAnalysis result = preAnalyzer.preAnalyze(plan);
4039
assertThat(plan.preAnalyzed(), is(true));
4140
assertThat(result.indices, hasSize(1));
4241
assertThat(result.indices.get(0).id().cluster(), is("elastic"));
4342
assertThat(result.indices.get(0).id().index(), is("index"));
44-
assertThat(result.indices.get(0).id().selector(), nullValue());
4543
}
4644

4745
public void testBasicIndexWithSelector() {
48-
LogicalPlan plan = new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, null, "index", "failures"), null, false);
46+
LogicalPlan plan = new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, null, "index::failures"), null, false);
4947
PreAnalysis result = preAnalyzer.preAnalyze(plan);
5048
assertThat(plan.preAnalyzed(), is(true));
5149
assertThat(result.indices, hasSize(1));
5250
assertThat(result.indices.get(0).id().cluster(), nullValue());
53-
assertThat(result.indices.get(0).id().index(), is("index"));
54-
assertThat(result.indices.get(0).id().selector(), is("failures"));
51+
assertThat(result.indices.get(0).id().index(), is("index::failures"));
5552
}
5653

5754
public void testComplicatedQuery() {
5855
LogicalPlan plan = new Limit(
5956
EMPTY,
6057
new Literal(EMPTY, 10, INTEGER),
61-
new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, null, "aaa", null), null, false)
58+
new UnresolvedRelation(EMPTY, new TableIdentifier(EMPTY, null, "aaa"), null, false)
6259
);
6360
PreAnalysis result = preAnalyzer.preAnalyze(plan);
6461
assertThat(plan.preAnalyzed(), is(true));

‎x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/parser/IdentifierBuilder.java‎

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,9 @@
77
package org.elasticsearch.xpack.sql.parser;
88

99
import org.antlr.v4.runtime.tree.ParseTree;
10+
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
1011
import org.elasticsearch.common.Strings;
12+
import org.elasticsearch.core.Tuple;
1113
import org.elasticsearch.xpack.ql.plan.TableIdentifier;
1214
import org.elasticsearch.xpack.ql.tree.Source;
1315
import org.elasticsearch.xpack.sql.parser.SqlBaseParser.IdentifierContext;
@@ -29,7 +31,56 @@ public TableIdentifier visitTableIdentifier(TableIdentifierContext ctx) {
2931
ParseTree tree = ctx.name != null ? ctx.name : ctx.TABLE_IDENTIFIER();
3032
String index = tree.getText();
3133

32-
return new TableIdentifier(source, visitIdentifier(ctx.catalog), unquoteIdentifier(index), visitIdentifier(ctx.selector));
34+
String cluster = visitIdentifier(ctx.catalog);
35+
String indexName = unquoteIdentifier(index);
36+
String selector = visitIdentifier(ctx.selector);
37+
38+
if (cluster != null && selector != null) {
39+
throw new ParsingException(
40+
source,
41+
"Invalid index name [{}:{}::{}], Selectors are not yet supported on remote cluster patterns",
42+
cluster,
43+
indexName,
44+
selector
45+
);
46+
}
47+
48+
if (selector != null) {
49+
try {
50+
IndexNameExpressionResolver.SelectorResolver.validateIndexSelectorString(indexName, selector);
51+
} catch (Exception e) {
52+
throw new ParsingException(source, e.getMessage());
53+
}
54+
}
55+
56+
if (indexName.contains(IndexNameExpressionResolver.SelectorResolver.SELECTOR_SEPARATOR)) {
57+
if (selector != null) {
58+
throw new ParsingException(
59+
source,
60+
"Invalid index name [{}::{}], Invalid usage of :: separator, only one :: separator is allowed per expression",
61+
indexName,
62+
selector
63+
);
64+
}
65+
try {
66+
Tuple<String, String> split = IndexNameExpressionResolver.splitSelectorExpression(indexName);
67+
indexName = split.v1();
68+
selector = split.v2();
69+
} catch (Exception e) {
70+
throw new ParsingException(source, e.getMessage());
71+
}
72+
if (selector != null) {
73+
try {
74+
IndexNameExpressionResolver.SelectorResolver.validateIndexSelectorString(indexName, selector);
75+
} catch (Exception e) {
76+
throw new ParsingException(source, "Invalid index name [{}::{}], {}", indexName, selector, e.getMessage());
77+
}
78+
}
79+
}
80+
81+
indexName = IndexNameExpressionResolver.combineSelectorExpression(indexName, selector);
82+
83+
return new TableIdentifier(source, cluster, indexName);
3384
}
3485

3586
@Override

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/parser/SqlParserTests.java‎

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -395,28 +395,82 @@ public void testQuotedIndexNameWithQuotedCluster() {
395395
public void testIndexNameDataSelector() {
396396
Project plan = project(parseStatement("SELECT * FROM foo::data"));
397397

398+
// data is effectively redundant, and so it is simplified to just the index name when executed
398399
assertThat(plan.child(), instanceOf(UnresolvedRelation.class));
399400
UnresolvedRelation relation = (UnresolvedRelation) plan.child();
400401
assertEquals("foo", relation.table().index());
401402
assertNull(relation.table().cluster());
402-
assertEquals("data", relation.table().selector());
403403
}
404404

405405
public void testIndexNameFailuresSelector() {
406406
Project plan = project(parseStatement("SELECT * FROM foo::failures"));
407407

408408
assertThat(plan.child(), instanceOf(UnresolvedRelation.class));
409409
UnresolvedRelation relation = (UnresolvedRelation) plan.child();
410-
assertEquals("foo", relation.table().index());
410+
assertEquals("foo::failures", relation.table().index());
411411
assertNull(relation.table().cluster());
412-
assertEquals("failures", relation.table().selector());
413412
}
414413

415-
public void testIndexNameClusterSeletorCombined() {
414+
public void testIndexNameClusterSelectorCombined() {
416415
ParsingException e = expectThrows(ParsingException.class, () -> parseStatement("SELECT * FROM cluster:foo::failures"));
416+
assertThat(
417+
e.getMessage(),
418+
containsString("Invalid index name [cluster:foo::failures], Selectors are not yet supported on remote cluster patterns")
419+
);
420+
}
421+
422+
public void testIndexNameInvalidSelector() {
423+
ParsingException e = expectThrows(ParsingException.class, () -> parseStatement("SELECT * FROM foo::bar"));
424+
assertThat(
425+
e.getMessage(),
426+
containsString("Invalid index name [foo::bar], invalid usage of :: separator, [bar] is not a recognized selector")
427+
);
428+
}
429+
430+
public void testIndexNameInvalidQuotedSelector() {
431+
ParsingException e = expectThrows(ParsingException.class, () -> parseStatement("SELECT * FROM \"foo::bar\""));
432+
assertThat(
433+
e.getMessage(),
434+
containsString("Invalid index name [foo::bar], invalid usage of :: separator, [bar] is not a recognized selector")
435+
);
436+
}
437+
438+
public void testIndexNameInvalidSelectors() {
439+
ParsingException e = expectThrows(ParsingException.class, () -> parseStatement("SELECT * FROM foo::bar::data"));
417440
assertThat(e.getMessage(), containsString("mismatched input '::' expecting {"));
418441
}
419442

443+
public void testIndexNameInvalidMixedQuotedSelectors() {
444+
ParsingException e = expectThrows(ParsingException.class, () -> parseStatement("SELECT * FROM \"foo::bar\"::data"));
445+
assertThat(
446+
e.getMessage(),
447+
containsString(
448+
"Invalid index name [foo::bar::data], Invalid usage of :: separator, only one :: separator is allowed per expression"
449+
)
450+
);
451+
}
452+
453+
public void testIndexNameInvalidInconsistentQuotedSelectors() {
454+
// We disallow this case because splicing escape quotes leads to too many corner cases
455+
ParsingException e = expectThrows(ParsingException.class, () -> parseStatement("SELECT * FROM \"foo::data,bar\"::data"));
456+
assertThat(
457+
e.getMessage(),
458+
containsString(
459+
"Invalid index name [foo::data,bar::data], Invalid usage of :: separator, only one :: separator is allowed per expression"
460+
)
461+
);
462+
}
463+
464+
public void testIndexNameInvalidQuotedSelectors() {
465+
ParsingException e = expectThrows(ParsingException.class, () -> parseStatement("SELECT * FROM \"foo::bar::data\""));
466+
assertThat(
467+
e.getMessage(),
468+
containsString(
469+
"Invalid index name [foo::bar::data], Invalid usage of :: separator, only one :: separator is allowed per expression"
470+
)
471+
);
472+
}
473+
420474
private LogicalPlan parseStatement(String sql) {
421475
return new SqlParser().createStatement(sql);
422476
}

‎x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelationTests.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,15 @@
2121
public class UnresolvedRelationTests extends ESTestCase {
2222
public void testEqualsAndHashCode() {
2323
Source source = new Source(between(1, 1000), between(1, 1000), randomAlphaOfLength(16));
24-
TableIdentifier table = new TableIdentifier(source, randomAlphaOfLength(5), randomAlphaOfLength(5), randomAlphaOfLength(5));
24+
TableIdentifier table = new TableIdentifier(source, randomAlphaOfLength(5), randomAlphaOfLength(5));
2525
String alias = randomBoolean() ? null : randomAlphaOfLength(5);
2626
String unresolvedMessage = randomAlphaOfLength(5);
2727
UnresolvedRelation relation = new UnresolvedRelation(source, table, alias, randomBoolean(), unresolvedMessage);
2828
List<Function<UnresolvedRelation, UnresolvedRelation>> mutators = new ArrayList<>();
2929
mutators.add(
3030
r -> new UnresolvedRelation(
3131
r.source(),
32-
new TableIdentifier(r.source(), r.table().cluster(), r.table().index() + "m", r.table().selector()),
32+
new TableIdentifier(r.source(), r.table().cluster(), r.table().index() + "m"),
3333
r.alias(),
3434
r.frozen(),
3535
r.unresolvedMessage()

0 commit comments

Comments
 (0)