Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
5 changes: 5 additions & 0 deletions docs/changelog/120277.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 120277
summary: Disallow CCS with lookup join
area: ES|QL
type: enhancement
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'd declare this as non-issue and would remove the changelog. It's not really an enhancement, anyway :)

issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import org.elasticsearch.compute.data.Block;
import org.elasticsearch.index.IndexMode;
import org.elasticsearch.logging.Logger;
import org.elasticsearch.transport.RemoteClusterAware;
import org.elasticsearch.xpack.core.enrich.EnrichPolicy;
import org.elasticsearch.xpack.esql.Column;
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
Expand Down Expand Up @@ -660,28 +659,6 @@ private Join resolveLookupJoin(LookupJoin join) {
return join;
}

// joining with remote cluster is not supported yet
if (join.left() instanceof EsRelation esr) {
for (var index : esr.index().concreteIndices()) {
if (index.indexOf(RemoteClusterAware.REMOTE_CLUSTER_INDEX_SEPARATOR) != -1) {
return join.withConfig(
new JoinConfig(
type,
List.of(
new UnresolvedAttribute(
join.source(),
"unsupported",
"LOOKUP JOIN does not support joining with remote cluster indices [" + esr.index().name() + "]"
)
),
List.of(),
List.of()
)
);
}
}
}

JoinType coreJoin = using.coreJoin();
// verify the join type
if (coreJoin != JoinTypes.LEFT) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
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 @@ -523,9 +524,15 @@ public PlanFactory visitJoinCommand(EsqlBaseParser.JoinCommandContext ctx) {
}

var target = ctx.joinTarget();
var rightPattern = visitIdentifier(target.index);

if (RemoteClusterAware.isRemoteIndexName(rightPattern)) {
throw new ParsingException(source(target), "LOOKUP JOIN does not support remote cluster indices [{}]", rightPattern);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: isn't this redundant, because below the p.forEachUp(UnresolvedRelation.class will walk over the join, too, and inspect the right hand side (which is an unresolved relation), anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Below p only references the left side. Possibly it should be renamed to make it a bit more obvious.
We could create a join and execute .forEachUp on it instead.
We might use current approach if we believe distinct messages make sense for left and right side.


UnresolvedRelation right = new UnresolvedRelation(
source(target),
new TableIdentifier(source(target.index), null, visitIdentifier(target.index)),
new TableIdentifier(source(target.index), null, rightPattern),
false,
emptyList(),
IndexMode.LOOKUP,
Expand All @@ -552,6 +559,18 @@ 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 -> {
if (RemoteClusterAware.isRemoteIndexName(r.table().index())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, I think there is a bug here: in case of multiple indices separated by commas, isRemoteIndexName is not appropriate because it treats the string as a single index name.

I think commas cannot be used inside date math expressions, and should thus be a correct separator to use to split r.table().index() back into parts. (Failing this, we'd have to go back to the step where the individual index patterns are parsed - but I don't think it should be necessary, given that we also use r.table().index() for the field caps calls.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, can we have : in date math expression? Otherwise presents of : in concatenated string should indicate remote was used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you can have : in date math. And our findings also were that commas are not used in date math, so you need to split by commas and then call isRemoteIndexName.

throw new ParsingException(
source(target),
"LOOKUP JOIN does not support remote cluster indices [{}]",
r.table().index()
);
}
});

return new LookupJoin(source, p, right, joinFields);
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2253,24 +2253,6 @@ public void testLookupJoinIndexMode() {
assertThat(e.getMessage(), containsString("1:25: invalid [test] resolution in lookup mode to an index in [standard] mode"));
}

public void testLookupJoinRemoteClusterUnsupported() {
assumeTrue("requires LOOKUP JOIN capability", EsqlCapabilities.Cap.JOIN_LOOKUP_V11.isEnabled());

var remoteIndexResolution = loadMapping("mapping-default.json", "remote:my-index");
var lookupResolution = AnalyzerTestUtils.defaultLookupResolution();
VerificationException e = expectThrows(
VerificationException.class,
() -> analyze(
"FROM remote:my-index | LOOKUP JOIN languages_lookup ON language_code",
AnalyzerTestUtils.analyzer(remoteIndexResolution, lookupResolution)
)
);
assertThat(
e.getMessage(),
containsString("1:24: LOOKUP JOIN does not support joining with remote cluster indices [remote:my-index]")
);
}

public void testImplicitCasting() {
var e = expectThrows(VerificationException.class, () -> analyze("""
from test | eval x = concat("2024", "-04", "-01") + 1 day
Expand Down
Loading