Skip to content

ESQL: Replace [ccq.mode] in favor of a policy prefix #105224

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.

Sign up for GitHub

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
elasticsearchmachine merged 3 commits into from
Feb 8, 2024
Merged

ESQL: Replace [ccq.mode] in favor of a policy prefix #105224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e6dd281
ESQL: Replace [ccq.mode] in favor of a policy prefix
costin Feb 7, 2024
9c28105
Fix typo
costin Feb 7, 2024
ec171d9
Merge branch 'main' into esql/eliminate-ccq-mode
costin Feb 8, 2024
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
6 changes: 3 additions & 3 deletions ...plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/EsqlTestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,11 +221,11 @@ public static String randomEnrichCommand(String name, Enrich.Mode mode, String m
}
withFields = " WITH " + String.join(",", fields);
}
String enrich = "ENRICH";
String enrich = "ENRICH ";
if (mode != Enrich.Mode.ANY || randomBoolean()) {
enrich += " [ccq.mode: " + mode + "] ";
enrich += " _" + mode + ":";
}
enrich += " " + name;
enrich += name;
enrich += onField;
enrich += withFields;
List<String> all = new ArrayList<>(before);
Expand Down
8 changes: 4 additions & 4 deletions ...c/internalClusterTest/java/org/elasticsearch/xpack/esql/action/CrossClustersEnrichIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ public void testTopNThenEnrichRemote() {
| %s
""", enrichHosts(Enrich.Mode.REMOTE));
var error = expectThrows(VerificationException.class, () -> runQuery(query).close());
assertThat(error.getMessage(), containsString("enrich with [ccq.mode:remote] can't be executed after LIMIT"));
assertThat(error.getMessage(), containsString("ENRICH with remote policy can't be executed after LIMIT"));
}

public void testLimitThenEnrichRemote() {
Expand All @@ -425,7 +425,7 @@ public void testLimitThenEnrichRemote() {
| %s
""", enrichHosts(Enrich.Mode.REMOTE));
var error = expectThrows(VerificationException.class, () -> runQuery(query).close());
assertThat(error.getMessage(), containsString("enrich with [ccq.mode:remote] can't be executed after LIMIT"));
assertThat(error.getMessage(), containsString("ENRICH with remote policy can't be executed after LIMIT"));
}

public void testAggThenEnrichRemote() {
Expand All @@ -438,7 +438,7 @@ public void testAggThenEnrichRemote() {
| sort vendor
""", enrichHosts(Enrich.Mode.ANY), enrichVendors(Enrich.Mode.REMOTE));
var error = expectThrows(VerificationException.class, () -> runQuery(query).close());
assertThat(error.getMessage(), containsString("enrich with [ccq.mode:remote] can't be executed after STATS"));
assertThat(error.getMessage(), containsString("ENRICH with remote policy can't be executed after STATS"));
}

public void testEnrichCoordinatorThenEnrichRemote() {
Expand All @@ -452,7 +452,7 @@ public void testEnrichCoordinatorThenEnrichRemote() {
var error = expectThrows(VerificationException.class, () -> runQuery(query).close());
assertThat(
error.getMessage(),
containsString("enrich with [ccq.mode:remote] can't be executed after another enrich with [ccq.mode:coordinator]")
containsString("ENRICH with remote policy can't be executed after another ENRICH with coordinator policy")
);
}

Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,8 @@ fragment ENRICH_POLICY_NAME_BODY
;

ENRICH_POLICY_NAME
: (LETTER | DIGIT) ENRICH_POLICY_NAME_BODY*
// allow prefix for the policy to specify its resolution
: (ENRICH_POLICY_NAME_BODY+ COLON)? ENRICH_POLICY_NAME_BODY+
Copy link
Copy Markdown
Member

@luigidellaquila luigidellaquila Feb 7, 2024

Choose a reason for hiding this comment

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

I guess changing the allowed characters here is on purpose.
Before this change, the policy name only allowed letters or numbers as first char, now any character apart from those explicitly excluded in ENRICH_POLICY_NAME_BODY is allowed.
The good is that it allows all the valid policy names to be used (eg. a policy name can start with $ or .)
The bad is that it also allows invalid policy names, eg. ., .. or names starting with +, _, -

The impact is limited though, because the validation takes care of checking that the policy does not exist.
Most of the problem is for Kibana, that will need further validation outside of the grammar

;

ENRICH_QUOTED_IDENTIFIER
Expand Down
6 changes: 1 addition & 5 deletions x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,13 +249,9 @@ showCommand
;

enrichCommand
: ENRICH setting* policyName=ENRICH_POLICY_NAME (ON matchField=qualifiedNamePattern)? (WITH enrichWithClause (COMMA enrichWithClause)*)?
: ENRICH policyName=ENRICH_POLICY_NAME (ON matchField=qualifiedNamePattern)? (WITH enrichWithClause (COMMA enrichWithClause)*)?
;

enrichWithClause
: (newName=qualifiedNamePattern ASSIGN)? enrichField=qualifiedNamePattern
;

setting
: OPENING_BRACKET name=SETTING COLON value=SETTING CLOSING_BRACKET
;
12 changes: 5 additions & 7 deletions x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,8 +387,8 @@ private static Failure validateUnsignedLongNegation(Neg neg) {
* TODO:
* For Limit and TopN, we can insert the same node after the remote enrich (also needs to move projections around)
* to eliminate this limitation. Otherwise, we force users to write queries that might not perform well.
* For example, `FROM test | ORDER @timestamp | LIMIT 10 | ENRICH[ccq.mode:remote]` doesn't work.
* In that case, users have to write it as `FROM test | ENRICH[ccq.mode:remote] | ORDER @timestamp | LIMIT 10`,
* For example, `FROM test | ORDER @timestamp | LIMIT 10 | ENRICH _remote:` doesn't work.
* In that case, users have to write it as `FROM test | ENRICH _remote: | ORDER @timestamp | LIMIT 10`,
* which is equivalent to bringing all data to the coordinating cluster.
* We might consider implementing the actual remote enrich on the coordinating cluster, however, this requires
* retaining the originating cluster and restructing pages for routing, which might be complicated.
Expand All @@ -409,15 +409,13 @@ private static void checkRemoteEnrich(LogicalPlan plan, Set<Failure> failures) {
}
if (u instanceof Enrich enrich && enrich.mode() == Enrich.Mode.REMOTE) {
if (limit[0]) {
failures.add(fail(enrich, "enrich with [ccq.mode:remote] can't be executed after LIMIT"));
failures.add(fail(enrich, "ENRICH with remote policy can't be executed after LIMIT"));
}
if (agg[0]) {
failures.add(fail(enrich, "enrich with [ccq.mode:remote] can't be executed after STATS"));
failures.add(fail(enrich, "ENRICH with remote policy can't be executed after STATS"));
}
if (enrichCoord[0]) {
failures.add(
fail(enrich, "enrich with [ccq.mode:remote] can't be executed after another enrich with [ccq.mode:coordinator]")
);
failures.add(fail(enrich, "ENRICH with remote policy can't be executed after another ENRICH with coordinator policy"));
}
}
});
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/EsqlBaseLexer.interp
View file
Open in desktop

Large diffs are not rendered by default.

Loading