-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Disallow CCS with lookup join #120277
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.
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
Merged
Merged
Disallow CCS with lookup join #120277
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
1b99a4d
Disallow CCS with lookup join
idegtiarenko b8cf1b3
ensure join target does not accept remote
idegtiarenko b1a0280
[CI] Auto commit changes from spotless
5f55608
inc lookup version
idegtiarenko 57a0df6
Revert "inc lookup version"
idegtiarenko c9b077e
Merge branch 'main' into es_10552
idegtiarenko c6be044
move check before index resolution
idegtiarenko 6a94eaf
Update docs/changelog/120277.yaml
idegtiarenko d283968
Delete docs/changelog/120277.yaml
idegtiarenko 66dd33c
Merge branch 'main' into es_10552
idegtiarenko 343d104
fix merge and resolve some comments
idegtiarenko 416a8fc
upd
idegtiarenko a6aa610
Merge branch 'main' into es_10552
idegtiarenko 59b78b7
fix merge
idegtiarenko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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 | ||
|
|
@@ -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)))); | ||
|
|
@@ -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)); | ||
|
|
@@ -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)))); | ||
|
|
@@ -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'"); | ||
| } | ||
|
|
||
|
|
@@ -2939,13 +2940,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).indexPattern().indexPattern(), 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).indexPattern().indexPattern(), equalTo(unquoteIndexPattern(basePattern))); | ||
|
|
@@ -2958,10 +2966,31 @@ 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 fromPatterns = randomIndexPatterns(without(CROSS_CLUSTER)); | ||
| var joinPattern = randomIndexPattern(CROSS_CLUSTER, without(WILDCARD_PATTERN)); | ||
| expectError( | ||
| "FROM " + fromPatterns + " | LOOKUP JOIN " + joinPattern + " ON " + randomIdentifier(), | ||
| "invalid index pattern [" + unquoteIndexPattern(joinPattern) + "], remote clusters are not supported in LOOKUP JOIN" | ||
| ); | ||
| } | ||
| { | ||
| // remote cluster on the left | ||
| var fromPatterns = randomIndexPatterns(CROSS_CLUSTER); | ||
| 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" | ||
| ); | ||
| } | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.