ESQL: use field_caps native nested fields filtering #117201

astefan · 2024-11-20T21:35:08Z

Fixes #117054 by partially reverting #111495 and using -nested as a filter for the _field_caps call.

…117054__fix

elasticsearchmachine · 2024-11-20T21:35:32Z

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

elasticsearchmachine · 2024-11-20T21:35:32Z

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

…into 117054__fix

…117054__fix

astefan · 2024-11-21T08:23:56Z

@elasticmachine update branch

elasticmachine · 2024-11-21T08:23:59Z

There are no new commits on the base branch.

astefan · 2024-11-21T08:25:05Z

@elasticmachine run elasticsearch-ci/bwc-snapshots

astefan · 2024-11-21T10:37:34Z

@elasticmachine update branch

bpintea

Simplifying.

bpintea · 2024-11-21T13:08:53Z

...sql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/FieldExtractorTestCase.java

+    /**
+     * https://github.com/elastic/elasticsearch/issues/117054
+     * One index with (Responses.process is nested; process.parent.command_line is supported):
+     * <pre>


Nit: I'd leave the mapping out of the javadoc, since its already legible in the code.

bpintea · 2024-11-21T13:12:52Z

...sql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/FieldExtractorTestCase.java

+        index("test", """
+            {"Responses.process.pid": 123,"process.parent.command_line":"run.bat"}""");
+
+        Map<String, Object> result = runEsql("FROM test");


Versions w/o this fix fail already at querying an empty index[*]. So a (totally optional) suggestion would be run a FROM test before indexing.

[*] On an 8.16:

java.lang.IllegalStateException: can't find input for [<no-fields>{r$}#26] at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planProject(LocalExecutionPlanner.java:602) at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:203) at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.planExchangeSink(LocalExecutionPlanner.java:321) at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:235) at org.elasticsearch.xpack.esql.planner.LocalExecutionPlanner.plan(LocalExecutionPlanner.java:174) at org.elasticsearch.xpack.esql.plugin.ComputeService.runCompute(ComputeService.java:446)

I've added the additional test. For the no-fields issue, likely it has something to do with an empty mapping. See #111545 where "empty mapping" has slightly different meanings depending on how the index is created and how field_caps behaves.

craigtaverner

LGTM - although I'd prefer the test comments to not have duplicates of the test json.

craigtaverner · 2024-11-21T13:28:08Z

...sql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/FieldExtractorTestCase.java

+    /**
+     * https://github.com/elastic/elasticsearch/issues/117054
+     * One index with (Responses.process is nested; process.parent.command_line is supported):
+     * <pre>


This big comment seems unnecessary because it duplicates the equally readable JSON in the test itself.

craigtaverner · 2024-11-21T13:33:32Z

...sql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/FieldExtractorTestCase.java

+
+        ResponseException e = expectThrows(ResponseException.class, () -> runEsql("FROM test | SORT Responses.process.pid"));
+        String err = EntityUtils.toString(e.getResponse().getEntity());
+        assertThat(err, containsString("line 1:18: Unknown column [Responses.process.pid]"));


Is this an error because ES|QL does not yet support nested queries, and if we did we'd need special syntax for it?

I expect we will indeed need a special syntax for it.

For now I think we should do our best to ignore nested fields. One day we will have to do something with them, but it'll be some kind of nested join syntax or something.

craigtaverner · 2024-11-21T13:34:27Z

...sql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/FieldExtractorTestCase.java

+     * Here is about the name "process" which is common to the two fields' hierarchies.
+     *
+     * test1
+     * <pre>


Since these two examples are reproduces in as readable a form in the test below, perhaps they should be removed from the comment, or replaced with a single-line text description instead.

craigtaverner · 2024-11-21T13:35:37Z

...sql/qa/server/src/main/java/org/elasticsearch/xpack/esql/qa/rest/FieldExtractorTestCase.java

+     * same.
+     *
+     * test1
+     * <pre>


Again, nicer to remove these.

nik9000

LGTM, though wait for the others to approve too I think. Sorry for making a mess here....

nik9000 · 2024-11-21T14:01:19Z

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

@@ -278,6 +259,8 @@ private static FieldCapabilitiesRequest createFieldCapsRequest(String index, Set
        // lenient because we throw our own errors looking at the response e.g. if something was not resolved
        // also because this way security doesn't throw authorization exceptions but rather honors ignore_unavailable
        req.indicesOptions(FIELD_CAPS_INDICES_OPTIONS);
+        // we ignore the nested data type fields starting with https://github.com/elastic/elasticsearch/pull/111495
+        req.filters("-nested");


Forgot to add a comment in the PR at this line; this is, indeed, the meat of the fix. I wasn't aware field_caps has -nested, learned yesterday about it. Very useful :-D.

…117054__fix

…into 117054__fix

craigtaverner · 2024-11-22T10:39:00Z

docs/changelog/117201.yaml

@@ -0,0 +1,6 @@
+pr: 117201
+summary: "ESQL: use `field_caps` native nested fields filtering"


Remove prefix.

docs/changelog/117201.yaml

astefan · 2024-11-22T10:49:37Z

@elasticmachine run elasticsearch-ci/bwc-snapshots

Co-authored-by: Craig Taverner <craig@amanzi.com>

…117054__fix

…into 117054__fix

…117054__fix

…ed as well

…117054__fix

elasticsearchmachine · 2024-11-22T18:50:06Z

💚 Backport successful

Status	Branch	Result
✅	8.x

* Just filter the nested fields natively with field_caps support --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Craig Taverner <craig@amanzi.com>

…117375) * ESQL: use field_caps native nested fields filtering (#117201) * Just filter the nested fields natively with field_caps support --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Craig Taverner <craig@amanzi.com> * Add import --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Craig Taverner <craig@amanzi.com>

* Just filter the nested fields natively with field_caps support --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Craig Taverner <craig@amanzi.com>

astefan added 3 commits November 20, 2024 23:21

Just filter the nested fields natively with field_caps support

fa2c94d

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

6ba152a

…117054__fix

Clean up comments

bb17078

astefan added >bug auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.17.0 labels Nov 20, 2024

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

Update docs/changelog/117201.yaml

4fac3ce

astefan requested a review from nik9000 November 20, 2024 21:35

elasticsearchmachine added v8.18.0 and removed v8.17.0 labels Nov 20, 2024

astefan added 4 commits November 21, 2024 09:59

Capability

f82531a

Merge branch '117054__fix' of https://github.com/astefan/elasticsearch …

e249f7d

…into 117054__fix

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

50a7b16

…117054__fix

Test fix

3960858

astefan requested a review from craigtaverner November 21, 2024 08:09

Merge branch 'main' into 117054__fix

52ebe0e

bpintea approved these changes Nov 21, 2024

View reviewed changes

craigtaverner approved these changes Nov 21, 2024

View reviewed changes

nik9000 approved these changes Nov 21, 2024

View reviewed changes

astefan added 2 commits November 22, 2024 12:25

Address reviews

9a70741

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

f56a338

…117054__fix

Merge branch '117054__fix' of https://github.com/astefan/elasticsearch …

45ba53e

…into 117054__fix

craigtaverner reviewed Nov 22, 2024

View reviewed changes

docs/changelog/117201.yaml Outdated Show resolved Hide resolved

astefan and others added 8 commits November 22, 2024 12:50

Update docs/changelog/117201.yaml

138e796

Co-authored-by: Craig Taverner <craig@amanzi.com>

bwc exclusion

791daae

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

2d4e31f

…117054__fix

Merge branch '117054__fix' of https://github.com/astefan/elasticsearch …

ff92659

…into 117054__fix

Another bwc exclusion

a7e4e5d

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

fc4fbdd

…117054__fix

Do the Capabilities exclusion differently so that BWC tests are cover…

c2a711c

…ed as well

Merge branch 'main' of https://github.com/elastic/elasticsearch into …

f404877

…117054__fix

astefan merged commit f3eb27e into elastic:main Nov 22, 2024
16 checks passed

astefan deleted the 117054__fix branch November 22, 2024 18:48

astefan mentioned this pull request Nov 22, 2024

[8.x] ESQL: use field_caps native nested fields filtering (#117201) #117375

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: use field_caps native nested fields filtering #117201

ESQL: use field_caps native nested fields filtering #117201

astefan commented Nov 20, 2024

elasticsearchmachine commented Nov 20, 2024

elasticsearchmachine commented Nov 20, 2024

astefan commented Nov 21, 2024

elasticmachine commented Nov 21, 2024

astefan commented Nov 21, 2024

astefan commented Nov 21, 2024

bpintea left a comment

bpintea Nov 21, 2024

bpintea Nov 21, 2024

astefan Nov 22, 2024

craigtaverner left a comment

craigtaverner Nov 21, 2024

craigtaverner Nov 21, 2024

nik9000 Nov 21, 2024

craigtaverner Nov 21, 2024

craigtaverner Nov 21, 2024

nik9000 left a comment

nik9000 Nov 21, 2024

astefan Nov 22, 2024

craigtaverner Nov 22, 2024

astefan commented Nov 22, 2024

elasticsearchmachine commented Nov 22, 2024

		@@ -0,0 +1,6 @@
		pr: 117201
		summary: "ESQL: use `field_caps` native nested fields filtering"

ESQL: use field_caps native nested fields filtering #117201

ESQL: use field_caps native nested fields filtering #117201

Conversation

astefan commented Nov 20, 2024

elasticsearchmachine commented Nov 20, 2024

elasticsearchmachine commented Nov 20, 2024

astefan commented Nov 21, 2024

elasticmachine commented Nov 21, 2024

astefan commented Nov 21, 2024

astefan commented Nov 21, 2024

bpintea 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

craigtaverner 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

nik9000 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

astefan commented Nov 22, 2024

elasticsearchmachine commented Nov 22, 2024

💚 Backport successful