Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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 @@ -11,10 +11,12 @@
import org.antlr.v4.runtime.Token;
import org.antlr.v4.runtime.tree.ParseTree;
import org.elasticsearch.Build;
import org.elasticsearch.common.Strings;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.dissect.DissectException;
import org.elasticsearch.dissect.DissectParser;
import org.elasticsearch.index.IndexMode;
import org.elasticsearch.transport.RemoteClusterAware;
import org.elasticsearch.xpack.esql.VerificationException;
import org.elasticsearch.xpack.esql.common.Failure;
import org.elasticsearch.xpack.esql.core.expression.Alias;
Expand Down Expand Up @@ -527,6 +529,13 @@ public PlanFactory visitJoinCommand(EsqlBaseParser.JoinCommandContext ctx) {
if (rightPattern.contains(WILDCARD)) {
throw new ParsingException(source(target), "invalid index pattern [{}], * is not allowed in LOOKUP JOIN", rightPattern);
}
if (RemoteClusterAware.isRemoteIndexName(rightPattern)) {
throw new ParsingException(
source(target),
"invalid index pattern [{}], remote clusters are not supported in LOOKUP JOIN",
rightPattern
);
}

UnresolvedRelation right = new UnresolvedRelation(
source(target),
Expand Down Expand Up @@ -557,6 +566,20 @@ public PlanFactory visitJoinCommand(EsqlBaseParser.JoinCommandContext ctx) {
throw new ParsingException(source, "JOIN ON clause only supports one field at the moment, found [{}]", matchFieldsCount);
}

return p -> new LookupJoin(source, p, right, joinFields);
return p -> {
p.forEachUp(UnresolvedRelation.class, r -> {
for (var leftPattern : Strings.splitStringByCommaToArray(r.table().index())) {
if (RemoteClusterAware.isRemoteIndexName(leftPattern)) {
throw new ParsingException(
source(target),
"invalid index pattern [{}], remote clusters are not supported in LOOKUP JOIN",
r.table().index()
);
}
}
});

return new LookupJoin(source, p, right, joinFields);
};
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One more test case suggestion: comma-separated list of index patterns, some of which are remote. E.g. FROM local,remote:pattern*,... with and without quoting.

Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@
import static org.elasticsearch.xpack.esql.EsqlTestUtils.paramAsIdentifier;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.paramAsPattern;
import static org.elasticsearch.xpack.esql.EsqlTestUtils.referenceAttribute;
import static org.elasticsearch.xpack.esql.IdentifierGenerator.Features.CROSS_CLUSTER;
import static org.elasticsearch.xpack.esql.IdentifierGenerator.Features.WILDCARD_PATTERN;
import static org.elasticsearch.xpack.esql.IdentifierGenerator.randomIndexPattern;
import static org.elasticsearch.xpack.esql.IdentifierGenerator.randomIndexPatterns;
Expand Down Expand Up @@ -307,18 +308,18 @@ public void testStatsWithoutGroups() {
);
}

public void testStatsWithoutAggs() throws Exception {
public void testStatsWithoutAggs() {
assertEquals(
new Aggregate(EMPTY, PROCESSING_CMD_INPUT, Aggregate.AggregateType.STANDARD, List.of(attribute("a")), List.of(attribute("a"))),
processingCommand("stats by a")
);
}

public void testStatsWithoutAggsOrGroup() throws Exception {
public void testStatsWithoutAggsOrGroup() {
expectError("from text | stats", "At least one aggregation or grouping expression required in [stats]");
}

public void testAggsWithGroupKeyAsAgg() throws Exception {
public void testAggsWithGroupKeyAsAgg() {
var queries = new String[] { """
row a = 1, b = 2
| stats a by a
Expand All @@ -339,7 +340,7 @@ public void testAggsWithGroupKeyAsAgg() throws Exception {
}
}

public void testStatsWithGroupKeyAndAggFilter() throws Exception {
public void testStatsWithGroupKeyAndAggFilter() {
var a = attribute("a");
var f = new UnresolvedFunction(EMPTY, "min", DEFAULT, List.of(a));
var filter = new Alias(EMPTY, "min(a) where a > 1", new FilteredExpression(EMPTY, f, new GreaterThan(EMPTY, a, integer(1))));
Expand All @@ -349,7 +350,7 @@ public void testStatsWithGroupKeyAndAggFilter() throws Exception {
);
}

public void testStatsWithGroupKeyAndMixedAggAndFilter() throws Exception {
public void testStatsWithGroupKeyAndMixedAggAndFilter() {
var a = attribute("a");
var min = new UnresolvedFunction(EMPTY, "min", DEFAULT, List.of(a));
var max = new UnresolvedFunction(EMPTY, "max", DEFAULT, List.of(a));
Expand Down Expand Up @@ -384,7 +385,7 @@ public void testStatsWithGroupKeyAndMixedAggAndFilter() throws Exception {
);
}

public void testStatsWithoutGroupKeyMixedAggAndFilter() throws Exception {
public void testStatsWithoutGroupKeyMixedAggAndFilter() {
var a = attribute("a");
var f = new UnresolvedFunction(EMPTY, "min", DEFAULT, List.of(a));
var filter = new Alias(EMPTY, "min(a) where a > 1", new FilteredExpression(EMPTY, f, new GreaterThan(EMPTY, a, integer(1))));
Expand Down Expand Up @@ -2067,41 +2068,41 @@ private void assertStringAsLookupIndexPattern(String string, String statement) {
assertThat(tableName.fold(FoldContext.small()), equalTo(string));
}

public void testIdPatternUnquoted() throws Exception {
public void testIdPatternUnquoted() {
var string = "regularString";
assertThat(breakIntoFragments(string), contains(string));
}

public void testIdPatternQuoted() throws Exception {
public void testIdPatternQuoted() {
var string = "`escaped string`";
assertThat(breakIntoFragments(string), contains(string));
}

public void testIdPatternQuotedWithDoubleBackticks() throws Exception {
public void testIdPatternQuotedWithDoubleBackticks() {
var string = "`escaped``string`";
assertThat(breakIntoFragments(string), contains(string));
}

public void testIdPatternUnquotedAndQuoted() throws Exception {
public void testIdPatternUnquotedAndQuoted() {
var string = "this`is`a`mix`of`ids`";
assertThat(breakIntoFragments(string), contains("this", "`is`", "a", "`mix`", "of", "`ids`"));
}

public void testIdPatternQuotedTraling() throws Exception {
public void testIdPatternQuotedTraling() {
var string = "`foo`*";
assertThat(breakIntoFragments(string), contains("`foo`", "*"));
}

public void testIdPatternWithDoubleQuotedStrings() throws Exception {
public void testIdPatternWithDoubleQuotedStrings() {
var string = "`this``is`a`quoted `` string``with`backticks";
assertThat(breakIntoFragments(string), contains("`this``is`", "a", "`quoted `` string``with`", "backticks"));
}

public void testSpaceNotAllowedInIdPattern() throws Exception {
public void testSpaceNotAllowedInIdPattern() {
expectError("ROW a = 1| RENAME a AS this is `not okay`", "mismatched input 'is' expecting {<EOF>, '|', ',', '.'}");
}

public void testSpaceNotAllowedInIdPatternKeep() throws Exception {
public void testSpaceNotAllowedInIdPatternKeep() {
expectError("ROW a = 1, b = 1| KEEP a b", "extraneous input 'b'");
}

Expand Down Expand Up @@ -2947,13 +2948,20 @@ public void testNamedFunctionArgumentWithUnsupportedNamedParameterTypes() {
}
}

public void testValidJoinPattern() {
public void testValidFromPattern() {
var basePattern = randomIndexPatterns();
var joinPattern = randomIndexPattern(without(WILDCARD_PATTERN));

var plan = statement("FROM " + basePattern);

assertThat(as(plan, UnresolvedRelation.class).table().index(), equalTo(unquoteIndexPattern(basePattern)));
}

public void testValidJoinPattern() {
var basePattern = randomIndexPatterns(without(CROSS_CLUSTER));
var joinPattern = randomIndexPattern(without(WILDCARD_PATTERN), without(CROSS_CLUSTER));
Comment on lines +2952 to +2953
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: this looks nice.

var onField = randomIdentifier();
var type = randomFrom("", "LOOKUP ");

var plan = statement("FROM " + basePattern + " | " + type + " JOIN " + joinPattern + " ON " + onField);
var plan = statement("FROM " + basePattern + " | LOOKUP JOIN " + joinPattern + " ON " + onField);

var join = as(plan, LookupJoin.class);
assertThat(as(join.left(), UnresolvedRelation.class).table().index(), equalTo(unquoteIndexPattern(basePattern)));
Expand All @@ -2966,10 +2974,30 @@ public void testValidJoinPattern() {
}

public void testInvalidJoinPatterns() {
var joinPattern = randomIndexPattern(WILDCARD_PATTERN);
expectError(
"FROM " + randomIndexPatterns() + " | JOIN " + joinPattern + " ON " + randomIdentifier(),
"invalid index pattern [" + unquoteIndexPattern(joinPattern) + "], * is not allowed in LOOKUP JOIN"
);
{
// wildcard
var joinPattern = randomIndexPattern(WILDCARD_PATTERN, without(CROSS_CLUSTER));
expectError(
"FROM " + randomIndexPatterns() + " | LOOKUP JOIN " + joinPattern + " ON " + randomIdentifier(),
"invalid index pattern [" + unquoteIndexPattern(joinPattern) + "], * is not allowed in LOOKUP JOIN"
);
}
{
// remote cluster on the right
var joinPattern = randomIndexPattern(CROSS_CLUSTER, without(WILDCARD_PATTERN));
expectError(
"FROM " + randomIndexPatterns() + " | LOOKUP JOIN " + joinPattern + " ON " + randomIdentifier(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: due to not excluding CCS in the FROM here and above, the test is actually slightly overconstrained, as an error message based on the usage of CCS in the FROM would also be correct.

That is to say, this test case passes because our implementation performs checks in a certain order, but ordered the other way around would also be correct.

"invalid index pattern [" + unquoteIndexPattern(joinPattern) + "], remote clusters are not supported in LOOKUP JOIN"
);
}
{
// remote cluster on the left
var fromPatterns = randomIndexPatterns(CROSS_CLUSTER, without(WILDCARD_PATTERN));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this would be more complete if the fromPatterns wouldn't exclude wildcard patterns. They should not affect the validation.

var joinPattern = randomIndexPattern(without(CROSS_CLUSTER), without(WILDCARD_PATTERN));
expectError(
"FROM " + fromPatterns + " | LOOKUP JOIN " + joinPattern + " ON " + randomIdentifier(),
"invalid index pattern [" + unquoteIndexPattern(fromPatterns) + "], remote clusters are not supported in LOOKUP JOIN"
);
}
}
}