ESQL: Quotations in index patterns allow for inconsistent pattern composition

[jbaiera]

ES|QL allows for using quotations in index pattern names to escape characters that might otherwise break the syntax. An example of this might be something like

FROM "this=that", """this[that"""

Additionally, we allow users to specify their entire index pattern in a string literal:

FROM "index1,index2,index3"

I've found recently that you can currently mix and match this syntax with cluster id syntax (and eventually index selector logic #120660) to create index patterns that probably should not be allowed.

All of the following patterns are valid in ES|QL, and parse to their unquoted equivalents when executing:

FROM cluster:"index1,index2"
# resolves to [cluster:index1, index2]

FROM cluster1:"index1,cluster2:index2"
# resolves to [cluster1:index1, cluster2:index2]

FROM "clusterA":"index1,index2,index3"
# resolves to [clusterA:index1, index2, index3]

When combining this existing logic with #120660 we end up with valid patterns like the following:

FROM "clusterA":"index1,index2,index3"::"data"
# resolves to [clusterA:index1, index2, index3::data]

FROM "cluster*":"index*,index*,index*::failures,*"::failures
# resolves to [cluster*:index*, index*, index*::failures, *::failures]

FROM "index1,index2,*"::failures
# resolves to [index1, index2, *::failures]

This seems like invalid pattern parsing to me. In my mind cluster:"index1,index2" implies that the comma is escaped, or that both indices should be resolved from the remote cluster provided. Since this incompatibility is really only possible under CCS, I would make a case to disallow commas in the quoted portions of ESQL that include an unquoted cluster id prefix (and in the future, an index selector suffix).

[elasticsearchmachine]

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

[nik9000]

AKA "jank combinations that make Nik cry"

[alex-spies]

This seems like invalid pattern parsing to me. In my mind cluster:"index1,index2" implies that the comma is escaped

++, esp. since multiple remotes are possible via from "cluster_one":*, "cluster_two":* .

I wonder if there's a good reason to interpret commas inside quotes as separators - but otherwise, I feel like this should be a bug.

[alex-spies]

There is related ongoing work for CCQ index pattern validation here: #122497

[jbaiera]

A suggestion for how to handle this going forward: It would be beneficial if we allowed quoted index patterns to be used with commas in two mutually exclusive ways. When searching multiple index patterns:

1. Each index pattern can be quoted individually to escape characters
   (like FROM "index1", "index2", "index3")
2. Or a list of indices can be provided in quotes, as long as there are no more expressions after the quoted list
   (like FROM "index1,index2,index3")

As mentioned, these cases should be mutually exclusive to one another.

Unfortunately, I don't think we'll be able to make this approach hold universally in ESQL without getting approval for a breaking change. Today we tolerate index patterns like FROM "index1,index2", index3, "index4". Changing this to throw an exception would break backwards compatibility. This is jank, but it's less janky than when we combine this kind of syntax with cluster names and selectors.

As such, I propose that we continue to support the previous GA usages where they occur, but throw a validation exception when that previous use is combined with cluster names and selectors outside of the quoted pattern:

Usage	Today's Result	Suggested Result
FROM "index1","index2"	✅ [index1, index2]	✅ [index1, index2]
FROM "index1,index2"	✅ [index1, index2]	✅ [index1, index2]
FROM "index1,index2", index3	✅ [index1, index2, index3]	❓ [index1, index2, index3] (Breaking change, should maybe disallow)
FROM cluster:"index1"	✅ [cluster:index1]	❓ [cluster:index1] (should maybe disallow)
FROM "cluster:index1"	✅ [cluster:index1]	✅ [cluster:index1]
FROM "cluster:index1", index2	✅ [cluster:index1, index2]	✅ [cluster:index1, index2]
FROM "cluster:index1", "index2"	✅ [cluster:index1, index2]	✅ [cluster:index1, index2]
FROM "cluster:index1,index2"	✅ [cluster:index1, index2]	✅ [cluster:index1, index2]
FROM cluster:"index1,index2"	✅ [cluster:index1, index2]	❌ disallow comma usage in index1,index2 with cluster name
FROM cluster:"index1,cluster:index2"	✅ [cluster:index1, cluster:index2]	❌ disallow comma usage in index1,cluster:index2 with cluster name

This would allow later operations with index selectors to mimic this behavior:

Usage	Without validation change	With validation change
FROM "index1", "index2::data"	✅ [index1, index2::data]	✅ [index1, index2::data]
FROM "index1,index2::data"	✅ [index1, index2::data]	✅ [index1, index2::data]
FROM "index1,index2::data", index3	✅ [index1, index2::data, index3]	✅ [index1, index2::data, index3]
FROM "index1,index2"::data	✅ [index1, index2::data]	❌ disallow comma usage in index1,index2 with selector provided
FROM "index1::data,index2"::data	✅ [index1::data, index2::data]	❌ disallow comma usage in index1::data,index2 with selector provided

Once CCS supports selectors, the patterns would be unified:

Usage	Without validation change	With validation change
FROM "cluster:index1", "index2::data"	✅ [cluster:index1, index2::data]	✅ [cluster:index1, index2::data]
FROM "cluster:index1,index2::data"	✅ [cluster:index1, index2::data]	✅ [cluster:index1, index2::data]
FROM "cluster:index1,index2::data", index3	✅ [cluster:index1, index2::data, index3]	✅ [cluster:index1, index2::data, index3]
FROM cluster:"index1,index2"::data	✅ [cluster:index1, index2::data]	❌ disallow comma usage in index1,index2 with cluster name and selector provided
FROM cluster:"index1::data,cluster:index2"::data	✅ [cluster:index1::data, cluster:index2::data]	❌ disallow comma usage in index1::data,cluster:index2 with cluster name and selector provided

[smalyshev]

I think we should either support "one quote to rule them all" (option 2) or "quote between commas" (option 1) but not other things, and not commas inside quotes. E.g. things like cluster:"index1" or "index1,index2", index3 shouldn't be allowed. Same for "index1,index2::data", index3, "cluster:index1,index2::data", index3 etc. It may be a bit of BC break, but it's better to take a bit of pain now than having to support increasingly complex code to deal with complications and inconsistencies for years to come, IMHO.

[jbaiera]

It was also brought to my attention by @alex-spies that we should probably disallow cluster:"index" as a format since that is also covered under the non-GA nature of CCS and thus would be reasonable to make a case for breaking that tolerance.

As for disallowing commas in quotation marks at all, I'm more on the fence about it. The documentation I can find lists quotation marks as the way to escape characters that are not accepted here by the ESQL parser, but there are plenty of special characters that shouldn't be escaped from index pattern parsing (like < and > for date math, : and :: for the different parts of the pattern, etc). I think it might be valuable to take a stance on what "escaping" means here: Does "index[1,index[2-<@now>" mean "two indices are resolved each named index[1 and index[2-2015-..." or does it mean "one index is trying to be resolved literally named index[1,index[2-<@now>? I don't think it's as simple to say we should escape commas inside of quotes when we don't escape other index syntax characters. Seems much easier to justify disallowing half-in-quotes, half-out-of-quotes index patterns.

[smalyshev]

As for disallowing commas in quotation marks at all, I'm more on the fence about it.

Maybe I went too far there - I don't necessarily mean banning commas in all cases, just not allowing it as semantically valid separator. I.e. "..." should be a single unit that is parsed as single index name, if we go option 1 route. I'm not sure if commas are necessary for datemath (they don't seem to have any special meaning and banned in index name so they can't be part of static string, and even if local date-time format has commas, I don't think you can have an index name that includes them?) but whatever is parsed by datemath should be happening inside quotes and it should be treated as single unit. I.e. index1, "index2, index3" or cluster:"index" shouldn't be a valid thing.

Seems much easier to justify disallowing half-in-quotes, half-out-of-quotes index patterns.

Agreed.

[jbaiera]

I updated my suggestion table to include the examples that are unclear if we should allow or not

[alex-spies]

@smalyshev , I think we should make a definitive decision on whether we should go forward with @jbaiera 's suggestion (I think we should), and to what extent.

@pawankartik-elastic 's PR #122497 seems to follow a different validation strategy, which treats FROM remote:"index, other_remote:other_index" equivalently to FROM "remote:index", "other_remote:other_index" and FROM remote:"index, other_index" equivalently to FROM "remote:index", "other_index" (if I'm not mistaken).

I think now would be a good time to settle on one strategy, so that #122497 can already follow it and we minimize potentially risky changes to the validation code of FROM.