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
ESQL: Introduce mode setting for ENRICH #103949
Changes from 2 commits
4f824fd
3115621
7cd037b
199c3c6
a679547
6d2dbd1
435d7ae
c37c228
1ff92a0
0e5eec4
7c1772a
baffff5
4b29105
6ef79de
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -246,9 +246,13 @@ showCommand | |
; | ||
|
||
enrichCommand | ||
: ENRICH policyName=fromIdentifier (ON matchField=qualifiedNamePattern)? (WITH enrichWithClause (COMMA enrichWithClause)*)? | ||
: ENRICH (enrichMode=enrichPolicyMode)? policyName=fromIdentifier (ON matchField=qualifiedNamePattern)? (WITH enrichWithClause (COMMA enrichWithClause)*)? | ||
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. Should the 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. That's the first option I went for however I wanted to be consistent with FROM namely have the options for a command specified after its declaration not per parameter, e.g.:
Another idea is to namespace the parameter to indicate what this mode is about since not everyone would be using CCQ and use something like ccq.mode, ccq.policy, ccq.policy.mode, ccq.policy.resolution:
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. Thanks for explaining. I am fine with either option. 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.
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. I like @nik9000's suggestion and I am kind of torn between the two approaches:
the drawback, though, is that if we add any additional elements to ENRICH in the future we need to do it the same natural language way, whereas with a square brackets approach the mechanism allows unlimited extensibility (just a made up setting name): Now, that I listed both options, I tend to favor the consistent square brackets option. 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. FWIW, I'd also find it more fluent keeping the consistency with the 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. FROM flows better however it's not a full property of ENRICH since it only makes sense when running a query in a CCQ environment against a CCQ index.
For these reasons I think we should give it less importance than a regular property. Essentially this introduces a slightly separate mode: metadata does NOT use We should align the two so either: 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.
I guess
👍
👍 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.
We haven't decided yet if we want to lenient or not:
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. Three options should work whether remote indices are present or not.
For local indices, all options have the same semantics. These options affect only the remote indices. |
||
; | ||
|
||
enrichWithClause | ||
: (newName=qualifiedNamePattern ASSIGN)? enrichField=qualifiedNamePattern | ||
; | ||
|
||
enrichPolicyMode | ||
: OPENING_BRACKET MODE ASSIGN FROM_UNQUOTED_IDENTIFIER CLOSING_BRACKET | ||
; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -717,6 +717,7 @@ static Enrich readEnrich(PlanStreamInput in) throws IOException { | |
return new Enrich( | ||
in.readSource(), | ||
in.readLogicalPlanNode(), | ||
in.readEnum(Enrich.Mode.class), | ||
in.readExpression(), | ||
in.readNamedExpression(), | ||
new EnrichPolicyResolution(in.readString(), new EnrichPolicy(in), IndexResolution.valid(readEsIndex(in))), | ||
|
@@ -727,6 +728,7 @@ static Enrich readEnrich(PlanStreamInput in) throws IOException { | |
static void writeEnrich(PlanStreamOutput out, Enrich enrich) throws IOException { | ||
out.writeNoSource(); | ||
out.writeLogicalPlanNode(enrich.child()); | ||
out.writeEnum(enrich.mode()); | ||
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. I think we need to handle BWC here. diff --git a/server/src/main/java/org/elasticsearch/TransportVersions.java b/server/src/main/java/org/elasticsearch/TransportVersions.java
index ce1f6d9ecaad..7d7adc4156de 100644
--- a/server/src/main/java/org/elasticsearch/TransportVersions.java
+++ b/server/src/main/java/org/elasticsearch/TransportVersions.java
@@ -178,6 +178,7 @@ public class TransportVersions {
public static final TransportVersion SNAPSHOTS_IN_PROGRESS_TRACKING_REMOVING_NODES_ADDED = def(8_566_00_0);
public static final TransportVersion SMALLER_RELOAD_SECURE_SETTINGS_REQUEST = def(8_567_00_0);
public static final TransportVersion UPDATE_API_KEY_EXPIRATION_TIME_ADDED = def(8_568_00_0);
+ public static final TransportVersion ESQL_ENRICH_MODE = def(8_569_00_0);
/*
* STOP! READ THIS FIRST! No, really,
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanNamedTypes.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanNamedTypes.java
index 7e81207ac919..e8c427870190 100644
--- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanNamedTypes.java
+++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanNamedTypes.java
@@ -7,6 +7,7 @@
package org.elasticsearch.xpack.esql.io.stream;
+import org.elasticsearch.TransportVersions;
import org.elasticsearch.common.TriFunction;
import org.elasticsearch.common.io.stream.NamedWriteable;
import org.elasticsearch.common.io.stream.StreamInput;
@@ -717,7 +718,7 @@ public final class PlanNamedTypes {
return new Enrich(
in.readSource(),
in.readLogicalPlanNode(),
- in.readEnum(Enrich.Mode.class),
+ in.getTransportVersion().onOrAfter(TransportVersions.ESQL_ENRICH_MODE) ? in.readEnum(Enrich.Mode.class) : Enrich.Mode.ANY,
in.readExpression(),
in.readNamedExpression(),
new EnrichPolicyResolution(in.readString(), new EnrichPolicy(in), IndexResolution.valid(readEsIndex(in))),
@@ -728,7 +729,11 @@ public final class PlanNamedTypes {
static void writeEnrich(PlanStreamOutput out, Enrich enrich) throws IOException {
out.writeNoSource();
out.writeLogicalPlanNode(enrich.child());
- out.writeEnum(enrich.mode());
+ if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_ENRICH_MODE)) {
+ out.writeEnum(enrich.mode());
+ } else {
+ throw new IllegalArgumentException("cluster is not ready to handle " + enrich.mode().name());
+ }
out.writeExpression(enrich.policyName());
out.writeNamedExpression(enrich.matchField());
out.writeString(enrich.policy().policyName()); 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. Thanks for confirming, I've added a similar fix. |
||
out.writeExpression(enrich.policyName()); | ||
out.writeNamedExpression(enrich.matchField()); | ||
out.writeString(enrich.policy().policyName()); | ||
|
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
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.
Nice