ESQL: Add OPTIONS clause to FROM command #106636

bpintea · 2024-03-21T22:37:11Z

This adds an OPTIONS clause to FROM command, allowing to specify search or index resolution options.
For now, the following options are supported:

preference
allow_no_indices
ignore_unavailable.

The OPTIONS clause take a comma-separated list of quoted AVPs. Example:

FROM index OPTIONS "preference"="_shards:1,2","allow_no_indices"="true"

This adds an OPTIONS clause to FROM, allowing to specify search or index resolution options, such as: preference, allow_no_indices or ignore_unavailable.

elasticsearchmachine · 2024-03-21T22:37:34Z

Hi @bpintea, I've created a changelog YAML for you.

bpintea · 2024-03-22T16:50:48Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ComputeService.java

+            requestFilter,
+            concreteIndices,
+            originalIndices,
+            esSourceOptions,


only added this parameter, diff grew because of breaking down of arguments line, due to line limit.

How about CCQ? Does it support having these preferences sent over?

Yes, they're part of the EsRelation, which is part of the Fragment which is sent across. I've added some REST tests and CSV test that's executed part of CCQ.

elasticsearchmachine · 2024-03-22T17:42:55Z

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

alex-spies

Looks good in general, thanks @bpintea !

Added a couple of remarks, many of which are minor. The major remarks are (summarized):

test coverage for the preferences options
docs update
wondering whether the right hand side of "some_option"="some_value" should be in quotes, even if it is expected to be e.g. a boolean
~~wondering whether we need to register warnings in case of e.g. a shard preference for a non-existent shard~~ @astefan chimed in and said this is consistent with what ES does in case of _search, so please disregard.

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

alex-spies · 2024-03-25T10:37:31Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

@@ -99,7 +99,20 @@ field
    ;

 fromCommand
-    : FROM fromIdentifier (COMMA fromIdentifier)* metadata?
+    : FROM fromIdentifier (COMMA fromIdentifier)* fromOptions? metadata?


I think we need to update the docs for FROM as well to reflect the grammar change.

I think we should mention in the docs that FROM can use backticks, e.g. in the edge case that an index is called options or metadata - but that's maybe out of scope for this PR.

FROM `metadata`,`options` ...

Yes, the docs will follow in separate PR -- I wanted to ensure we want to keep the format.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Analyzer.java

alex-spies · 2024-03-25T10:57:44Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/io/stream/PlanNamedTypes.java

    }

    static void writeEsRelation(PlanStreamOutput out, EsRelation relation) throws IOException {
        assert relation.children().size() == 0;
        out.writeNoSource();
        writeEsIndex(out, relation.index());
        writeAttributes(out, relation.output());
+        if (out.getTransportVersion().onOrAfter(TransportVersions.ESQL_ES_SOURCE_OPTIONS)) {
+            if (relation.esSourceOptions() != null) {
+                out.writeBoolean(true);


Is marking the existence of the source option with a boolean really necessary?
We encode the options as 3 optional strings; that handles the optionality already, why repeat this logic?

Is marking the existence of the source option with a boolean really necessary? We encode the options as 3 optional strings

Without this boolean, how would the remote side know if it should attempt to read three (optional) strings or not? Reading an optional strings involves consuming at least a boolean.

What I meant to say: StreamOutput.writeOptionalString already prepends a boolean to say "there's a string!" or "there's no string!". We're doubling this logic.

The serialization code in this PR, however, optimizes for the (common?) case where there's no options. My point is that this is unnecessary, because we'd only be sending 1 byte (false) instead of 3 bytes (false, false, false), at the cost of slightly more complex code (additional null-handling at the level of EsSourceOptions).

I don't feel strongly about this, but I'd prefer if

the EsSourceOptions were not nullable (each of the options that are encoded in them is nullable already, so there's two ways to express that no options were set)

and if the serialization always sent the EsSourceOptions as 3 optional strings, rather than specialcasing for when the EsSourceOptions is null.

Got it, you were still addressing the nullability of the EsSourceOptions instance. Ok, took on your suggestion and applied it.

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/analyzer/TableInfo.java

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/options/EsSourceOptions.java

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

alex-spies · 2024-03-25T11:47:06Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

+    ;
+
+configOption
+    : string ASSIGN string


Hmm, it's a bit unfortunate that this leads to quotes around e.g. booleans. In particular, one has to write ... OPTIONS "allow_no_indices"="true" instead of "allow_no_indices"=true.

Indeed, ideally one would simply write smth like OPTIONS allow_no_indices=true.
But I think the rational was that (1) it might require explaining when to use value quotes and when not (not that difficult, but would come with some load) and (2) that we can relax the quoting later, if needed.

alex-spies · 2024-03-25T11:54:22Z

...ugin/esql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/RestEsqlTestCase.java

+        assertEquals(expectedTextBody("txt", count, null), runEsqlAsTextWithFormat(builder.apply(null), "txt", null));
+
+        // returns nothing (0 for count), given the non-existing shard as preference
+        String option = "\"preference\"=\"_shards:666\"";


Shouldn't we add more tests that demonstrate that the other possible preference options work correctly?

We treat the values in a opaque way and this tests that these "makes it through". Testing it exhaustively involves multiple nodes, which seemed an overkill just for this feature (and I assume there are already other tests for all these values).

astefan

Review in progress. Found that FROM employees OPTIONS "preference"="" (empty string) returns org.elasticsearch.ElasticsearchException$1: Index 0 out of bounds for length 0

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/options/EsSourceOptions.java

astefan · 2024-03-25T13:48:00Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/options/EsSourceOptions.java

+
+    public void addOption(String name, String value) {
+        switch (name) {
+            case "allow_no_indices" -> {


You could take the name of this option, at least, from IndicesOptions.WildcardOptions.ALLOW_NO_INDICES and use this.

Good idea, fixed.
(I started from "preference", which is used as hardcoded string everywhere, so didn't explore further.)

astefan · 2024-03-25T13:49:32Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/options/EsSourceOptions.java

+                }
+                preference = value;
+            }
+            default -> throw new IllegalArgumentException("unknown option named [" + name + "]");


How about offering some suggestions? StringUtils.findSimilar

Good idea, added.

astefan · 2024-03-25T13:58:52Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/parser/StatementParserTests.java

+    public void testFromOptionsInvalidIndicesOptionValue() {
+        expectError(
+            FROM + " options \"allow_no_indices\"=\"foo\"",
+            "line 1:20: invalid options provided: Could not convert [allow_no_indices] to boolean"


I think here we can be more strict. From my tests, we only allow true and false (not even uppercase versions of them). So, unless we offer more options (like, t/f, 1/0, TRUE/FALSE) and I believe we shouldn't and we must follow the same behavior ES does, we can improve this message (and potentially use the same ES generates). ES itself provides more info:

"error": { "root_cause": [ { "type": "illegal_argument_exception", "reason": "Could not convert [allow_no_indices] to boolean" } ], "type": "illegal_argument_exception", "reason": "Could not convert [allow_no_indices] to boolean", "caused_by": { "type": "illegal_argument_exception", "reason": "Failed to parse value [TRUE] as only [true] or [false] are allowed." }

Thanks.
The parsing of the options is delegated to IndicesOptions, so we follow those rules. But forgot to pipe the cause through, it's now fixed. And I also incorporated your later observation, though IMO this unpacking of the causes should be left to the client (but maybe here it's OK to do it).

astefan · 2024-03-25T14:14:04Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plugin/ComputeService.java

+            requestFilter,
+            concreteIndices,
+            originalIndices,
+            esSourceOptions,


How about CCQ? Does it support having these preferences sent over?

astefan

LGTM

I've added one suggestion and, also, I would be more comfortable if there would be a csv-spec test, as well. And here I am more thinking about the mixed-cluster and multi-clusters IT tests (the csv tests are used there as well; with EsqlSpecTestCase base class). RestEsqlTestCase (the one you have in PR) is for single-node environment only.

astefan · 2024-03-27T10:50:46Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/LogicalPlanBuilder.java

@@ -220,7 +220,7 @@ public LogicalPlan visitFromCommand(EsqlBaseParser.FromCommandContext ctx) {
                try {
                    esSourceOptions.addOption(name, value);
                } catch (IllegalArgumentException iae) {
-                    throw new ParsingException(source(nameContext), "invalid options provided: " + iae.getMessage());
+                    throw new ParsingException(iae, source(nameContext), "invalid options provided: " + iae.getMessage());


What I meant with my previous suggestion was something like this:

Suggested change

throw new ParsingException(iae, source(nameContext), "invalid options provided: " + iae.getMessage());

var cause = iae.getCause() != null ? "; " + iae.getCause().getMessage() : "";

throw new ParsingException(iae, source(nameContext), "invalid options provided: " + iae.getMessage() + cause);

to have an error message like "reason": "line 1:56: invalid options provided: Could not convert [allow_no_indices] to boolean; Failed to parse value [TRUE] as only [true] or [false] are allowed.",

alex-spies

LGTM, left only minor remarks. Thanks @bpintea !

alex-spies · 2024-03-28T10:03:39Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseLexer.g4

@@ -186,8 +186,10 @@ FROM_OPENING_BRACKET : OPENING_BRACKET -> type(OPENING_BRACKET);
 FROM_CLOSING_BRACKET : CLOSING_BRACKET -> type(CLOSING_BRACKET);
 FROM_COMMA : COMMA -> type(COMMA);
 FROM_ASSIGN : ASSIGN -> type(ASSIGN);
+FROM_QUTED_STRING : QUOTED_STRING -> type(QUOTED_STRING);


Suggested change

FROM_QUTED_STRING : QUOTED_STRING -> type(QUOTED_STRING);

FROM_QUOTED_STRING : QUOTED_STRING -> type(QUOTED_STRING);

alex-spies · 2024-03-28T10:06:36Z

x-pack/plugin/esql/src/main/antlr/EsqlBaseParser.g4

@@ -99,7 +99,20 @@ field
    ;

 fromCommand
-    : FROM fromIdentifier (COMMA fromIdentifier)* metadata?
+    : FROM fromIdentifier (COMMA fromIdentifier)* fromOptions? metadata?


I just realized, it will not be possible to write FROM index METADATA ... OPTIONS ... because we only allow the fixed order FROM->OPTIONS->METADATA.

Probably okay for now, but we might want to generalize that later.

Yes, I had that thought too (it's just a grammar line more). But had a look at SQL (which doesn't allow this swapping where it could) and thought of how we now request quotes (i.e. no flexibility) and thought I'd keep it inline.

I missed this - I'm fine with the order being fixed however I would keep the metadata? first, e.g.:
FROM index METADATA ... OPTIONS... since in general I expect OPTIONS to be the last item in a command mainly because there can be a long list of parameters and because METADATA and others could be considered specialized OPTIONS and thus should take precedence.

alex-spies · 2024-03-28T10:14:55Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/floats.csv-spec

+convertFromDatetimeWithOptions#[skip:-8.13.99, reason:OPTIONS added in 8.14]
+  FROM employees OPTIONS "allow_no_indices"="false","preference"="_shards:0"


I wonder if floats.csv-spec is the best place for this; we're adding the test to test the OPTIONS, no? Maybe it's time for a from.csv-spec? This could be consolidated with id.csv-spec.

Good suggestion, thanks.

alex-spies · 2024-03-28T10:25:51Z

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/options/EsSourceOptions.java

+                List<String> matches = StringUtils.findSimilar(name, List.of(ALLOW_NO_INDICES, IGNORE_UNAVAILABLE, OPTION_PREFERENCE));
+                if (matches.isEmpty() == false) {
+                    String suggestions = matches.size() == 1 ? "[" + matches.get(0) + "]" : "any of " + matches;
+                    message += ", did you mean " + suggestions;


Suggested change

message += ", did you mean " + suggestions;

message += ", did you mean " + suggestions + "?";

Add OPTIONS clause to FROM command

06ddce5

This adds an OPTIONS clause to FROM, allowing to specify search or index resolution options, such as: preference, allow_no_indices or ignore_unavailable.

bpintea added >enhancement :Analytics/ES|QL AKA ESQL ES|QL-ui Impacts ES|QL UI v8.14.0 labels Mar 21, 2024

bpintea and others added 3 commits March 21, 2024 23:37

Update docs/changelog/106636.yaml

2630e9a

Extend options usage to field caps

c3a6482

Add rest tests for the supported options

7a6df1c

bpintea commented Mar 22, 2024

View reviewed changes

bpintea marked this pull request as ready for review March 22, 2024 17:42

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Mar 22, 2024

costin requested review from astefan, alex-spies and fang-xing-esql March 22, 2024 22:00

alex-spies reviewed Mar 25, 2024

View reviewed changes

astefan reviewed Mar 25, 2024

View reviewed changes

x-pack/plugin/ql/src/main/java/org/elasticsearch/xpack/ql/options/EsSourceOptions.java Show resolved Hide resolved

astefan reviewed Mar 25, 2024

View reviewed changes

Address review comments

2aa0e34

This was referenced Mar 27, 2024

ESQL: allow returning an empty response if ignore_unavailable is used for non-existent indices #106805

Open

[ES|QL] - Using a wildcard "*" on a none existing index together with a existing index, then the user doesn't get an error from ES #104987

Open

astefan approved these changes Mar 27, 2024

View reviewed changes

bpintea added 3 commits March 27, 2024 23:02

More tests. More review comments

533292e

Merge branch 'main' into esql/from_options

d7b47c1

Spotless

da19b7b

alex-spies reviewed Mar 28, 2024

View reviewed changes

bpintea added 2 commits March 28, 2024 14:25

Merge branch 'main' into esql/from_options

7045e10

Address review comments

5d32262

Use cluster features in tests. Address comments

52c3045

bpintea merged commit aeeb597 into elastic:main Mar 28, 2024
14 checks passed

bpintea deleted the esql/from_options branch March 28, 2024 17:07

bpintea mentioned this pull request Apr 15, 2024

ESQL: Add configuration-like property to FROM command #106449

Open

bpintea mentioned this pull request May 15, 2024

ESQL: Remove OPTIONS clause in FROM command #108692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Add OPTIONS clause to FROM command #106636

ESQL: Add OPTIONS clause to FROM command #106636

bpintea commented Mar 21, 2024 •

edited

elasticsearchmachine commented Mar 21, 2024

bpintea Mar 22, 2024

astefan Mar 25, 2024

bpintea Mar 27, 2024

elasticsearchmachine commented Mar 22, 2024

alex-spies left a comment •

edited

alex-spies Mar 25, 2024

bpintea Mar 27, 2024

alex-spies Mar 25, 2024

bpintea Mar 27, 2024

alex-spies Mar 28, 2024

bpintea Mar 28, 2024

alex-spies Mar 25, 2024

bpintea Mar 27, 2024

alex-spies Mar 25, 2024

bpintea Mar 27, 2024

astefan left a comment

astefan Mar 25, 2024

bpintea Mar 27, 2024

astefan Mar 25, 2024

bpintea Mar 27, 2024

astefan Mar 25, 2024

bpintea Mar 27, 2024

astefan Mar 25, 2024

astefan left a comment

astefan Mar 27, 2024

alex-spies left a comment

alex-spies Mar 28, 2024

alex-spies Mar 28, 2024

bpintea Mar 28, 2024

costin Mar 29, 2024 •

edited

alex-spies Mar 28, 2024

bpintea Mar 28, 2024

alex-spies Mar 28, 2024

	throw new ParsingException(iae, source(nameContext), "invalid options provided: " + iae.getMessage());
	var cause = iae.getCause() != null ? "; " + iae.getCause().getMessage() : "";
	throw new ParsingException(iae, source(nameContext), "invalid options provided: " + iae.getMessage() + cause);

	FROM_QUTED_STRING : QUOTED_STRING -> type(QUOTED_STRING);
	FROM_QUOTED_STRING : QUOTED_STRING -> type(QUOTED_STRING);

		convertFromDatetimeWithOptions#[skip:-8.13.99, reason:OPTIONS added in 8.14]
		FROM employees OPTIONS "allow_no_indices"="false","preference"="_shards:0"

	message += ", did you mean " + suggestions;
	message += ", did you mean " + suggestions + "?";

ESQL: Add OPTIONS clause to FROM command #106636

ESQL: Add OPTIONS clause to FROM command #106636

Conversation

bpintea commented Mar 21, 2024 • edited

elasticsearchmachine commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Mar 22, 2024

alex-spies left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin Mar 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpintea commented Mar 21, 2024 •

edited

alex-spies left a comment •

edited

costin Mar 29, 2024 •

edited