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

[costin]

For consistency, replace [ccq.mode:] with _:policyName ENRICH [ccq.mode=any] policyName becomes ENRICH _any:policyName

[elasticsearchmachine]

Pinging @elastic/es-analytical-engine (Team:Analytics)

[luigidellaquila]

Technically the PR looks good.

From a usability point of view I have some doubts, in particular for the syntax mode:policy_name clashing with CCQ syntax cluster_name:index_name.

This feature is specifically for CCQ, so users will easily mix the two things and try ENRICH cluster_name:policy_name, that would also make a lot of sense as an option to explicitly choose which cluster/enrich index you want to use.

If that's in the radar, then we have to take into consideration the fact that cluster names could clash with the keywords we are defining now for CCQ mode (_local, _coordinator, _any).

IMHO this aspect has to be clarified before merging

[luigidellaquila]

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

[astefan]

If that's in the radar, then we have to take into consideration the fact that cluster names could clash with the keywords we are defining now for CCQ mode (_local, _coordinator, _any)

Digging a bit in the code and performing some tests, _ (underscore) is allowed as a cluster alias. If I am not mistaken this is the regex that is checked for matches for the cluster alias name: ((\Qcluster.remote.\E)([-\w]+)\.\Qseeds\E)(?:\..*)? (this is cluster.remote.[alias_name].seeds configuration from docs) where \w definitely allows underscore. And a simple test with PUT /_cluster/settings allows _any or ___123 as an alias name.

If we will consider in the future to support ENRICH my_cluster_alias:policy_name then a cluster named _any would be ambiguous.

[costin]

Enrich policies follow the same rules as an index - see

elasticsearch/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichStore.java

Lines 63 to 69 in f26691f

	}
	// The policy name is used to create the enrich index name and
	// therefor a policy name has the same restrictions as an index name
	MetadataCreateIndexService.validateIndexOrAliasName(
	name,
	(policyName, error) -> new IllegalArgumentException("Invalid policy name [" + policyName + "], " + error)
	);

which leads to

elasticsearch/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java

Lines 231 to 233 in f26691f

	if (index.charAt(0) == '_' || index.charAt(0) == '-' || index.charAt(0) == '+') {
	throw exceptionCtor.apply(index, "must not start with '_', '-', or '+'");
	}

Essentially _ cannot be used be used, which is consistent with the index conventions so _any, _coordinator and _remote should be safe.

The proposed grammar changes overload the meaning of : when resolving an enrich policy across CCQ.
I've checked with Nhat to confirm it makes little sense to refer to a policy from a remote cluster - it's more likely that if such ability is created, it would be done inside the policy itself on each individual cluster.
That is ENRICH remote-cluster:some-policy is highly unlikely at the moment.
Assuming it will, in that scenario one way to disambiguate is to use double qualifiers:
ENRICH _any:remote-cluster:some-policy or come up at the time with another character.

I propose to mark the feature as experimental - which we should do for CCQ as well since it has a different lifecycle than the rest of ESQL.

[astefan]

I've checked with Nhat to confirm it makes little sense to refer to a policy from a remote cluster - it's more likely that if such ability is created, it would be done inside the policy itself on each individual cluster.
That is ENRICH remote-cluster:some-policy is highly unlikely at the moment.
Assuming it will, in that scenario one way to disambiguate is to use double qualifiers:
ENRICH _any:remote-cluster:some-policy or come up at the time with another character.

I am ok with the change given the fairly certain aspect of cluster name aliases not being used in the future or being integrated differently.

I propose to mark the feature as experimental - which we should do for CCQ as well since it has a different lifecycle than the rest of ESQL.

👍

[bpintea]

Indeed, there's some room for initial confusion, but IMO the fact that there's no way to accidentally execute the query in an unexpected way contains it. Once you hit the first error (and dip in the documentation, which will generally have to happen before executing an ENRICH, whose policy setting up is a more involved process) the user should be set on the right path.

I don't find the syntax involving a prefixing _ particularly pretty (even after understanding its necessity).

I do find the existing ENRICH <config> <policy> ... syntax better overall (and inline with other similar languages), but not having a better alternative when:

• having to drop the [ ] usage;
• not wanting to introduce another keyword, which would break the command's flow.

So will 👍 and maybe we can still iterate, if marking it as experimental.

[luigidellaquila]

I propose to mark the feature as experimental - which we should do for CCQ as well since it has a different lifecycle than the rest of ESQL.

With this assumption LGTM, we'll have time to see if it's a real UX problem (and in case we'll have a little margin to fix it) or if we are just overestimating its impact