ESQL: Remove the `NESTED` DataType #111495

nik9000 · 2024-07-31T20:10:59Z

We don't support nested fields at the moment and when we do I'm not sure it'll be a DataType. Maybe. Probably. But let's deal with that when we support it.

We don't support nested fields at the moment and when we do I'm not sure it'll *be* a DataType. Maybe. Probably. But let's deal with that when we support it.

nik9000 · 2024-07-31T20:11:48Z

x-pack/plugin/esql/qa/testFixtures/src/main/java/org/elasticsearch/xpack/esql/LoadMapping.java

@@ -82,7 +81,7 @@ private static void walkMapping(String name, Object value, Map<String, EsField>
            // extract field type
            DataType esDataType = getType(content);
            final Map<String, EsField> properties;
-            if (esDataType == OBJECT || esDataType == NESTED) {
+            if (esDataType == OBJECT) {


This is from Types in QL, but it's only used in testing in ESQL. I think this safe for now.

nik9000 · 2024-07-31T20:14:44Z

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

-                // allow compound object even if they are unknown (but not NESTED)
-                if (type != NESTED && fieldProperties.isEmpty() == false) {
+                // allow compound object even if they are unknown
+                if (fieldProperties.isEmpty() == false) {


This is the most interesting line, I think. But we don't make NESTED any more so I feel like this is pretty safe.

So this would change things. It'd make the sub-fields of the NESTED object appear. I think. I'll double check.

Yeah, I think this is ok. Stuff like object with sub-fields still work 👍 ; I tested an empty index with this kind of mapping:

"name": { "type": "object", "properties": { "first_name": { "type": "keyword" }, "last_name": { "type": "keyword" } } }

and we get back only name.first_name and name.last_name. Which I think it's ok? Now I'm wondering if OBJECT has the same treatment as NESTED, since it disappears.

Also, nested doesn't seem to accept fields, but what it does (and I forgot about it) is include_in_parent but I don't think we ever supported this since _field_caps behaves identically with include_in_parent true or false.

elasticsearchmachine · 2024-07-31T20:15:30Z

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

nik9000 · 2024-08-01T15:25:18Z

I talked with @astefan and I'm going to make a yaml test that uses nested just to make sure we're not doing something really unexpected. It should be fine, but let's be super careful here.

nik9000 · 2024-08-01T16:22:29Z

Mechanically what happens for nested fields and all fields under them is that the IndexResolver hits the nested field and marks it as unsupported. Then it hits the children of the nested field and marks them as unsupported because the parent is unsupported.

nik9000 · 2024-08-01T16:55:57Z

Mechanically what happens for nested fields and all fields under them is that the IndexResolver hits the nested field and marks it as unsupported. Then it hits the children of the nested field and marks them as unsupported because the parent is unsupported.

We don't want this. At least, it's a change and maybe not a good one. nested fields aren't the same as non-nested fields. Previously we were removing them entirely from ESQL. It just didn't see them. With this change as it stands now they'd start showing up as nested. I don't think we want that, at least, not now. I'm going to try and filter them out earlier.

nik9000 · 2024-08-01T17:09:40Z

OK! I've moved the filtering of nested fields lower. That should be better....

astefan

I think it's ok to remove this data type and I hope we are careful enough with this step.
Maybe add two-three more to 40_unsupported_types.yml with a mapping that more paranoid:

            "name": {
                "type": "object",
                "properties": {
                    "first_name": {
                        "type": "keyword"
                    },
                    "last_name": {
                        "type": "keyword"
                    }
                }
            },
            "bla": {
                "type": "object",
                "properties": {
                    "obj": {
                        "type": "object",
                        "properties": {
                            "first_name": {
                                "type": "keyword"
                            },
                            "last_name": {
                                "type": "keyword"
                            }
                        }
                    }
                }
            },
            "nested": {
                "type": "nested",
                "properties": {
                    "sub_nested": {
                        "type": "nested",
                        "properties": {
                            "first_name": {
                                "type": "keyword"
                            }
                        }
                    },
                    "last_name": {
                        "type": "keyword"
                    }
                }
            }

astefan · 2024-08-02T14:57:16Z

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

-                // allow compound object even if they are unknown (but not NESTED)
-                if (type != NESTED && fieldProperties.isEmpty() == false) {
+                // allow compound object even if they are unknown
+                if (fieldProperties.isEmpty() == false) {


Yeah, I think this is ok. Stuff like object with sub-fields still work 👍 ; I tested an empty index with this kind of mapping:

"name": { "type": "object", "properties": { "first_name": { "type": "keyword" }, "last_name": { "type": "keyword" } } }

and we get back only name.first_name and name.last_name. Which I think it's ok? Now I'm wondering if OBJECT has the same treatment as NESTED, since it disappears.

Also, nested doesn't seem to accept fields, but what it does (and I forgot about it) is include_in_parent but I don't think we ever supported this since _field_caps behaves identically with include_in_parent true or false.

nik9000 · 2024-08-02T19:01:12Z

Maybe add two-three more to 40_unsupported_types.yml with a mapping that more paranoid:

👍

nik9000 · 2024-08-05T18:57:10Z

That snapshot failures are real - old versions sometimes send nested as the data type:

[2024-08-05T12:22:54,015][WARN ][o.e.x.e.a.EsqlResponseListener] [test-cluster-0] Request failed with status [INTERNAL_SERVER_ERROR]: java.io.IOException: Unknown DataType for type name: nested
	at org.elasticsearch.xpack.esql.core.type.DataType.readFrom(DataType.java:449)
	at org.elasticsearch.xpack.esql.core.type.EsField.<init>(EsField.java:57)
	at org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput.readNamedWriteable(NamedWriteableAwareStreamInput.java:56)
	at org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput.readNamedWriteable(NamedWriteableAwareStreamInput.java:32)
	at org.elasticsearch.xpack.esql.io.stream.PlanNamedTypes.lambda$readEsIndex$3(PlanNamedTypes.java:771)
	at org.elasticsearch.common.io.stream.StreamInput.readImmutableMap(StreamInput.java:
	at org.elasticsearch.xpack.esql.io.stream.PlanNamedTypes.readEsIndex(PlanNamedTypes.java:771)
	at org.elasticsearch.xpack.esql.io.stream.PlanNamedTypes.readEsRelation(PlanNamedTypes.java:545)

When we serialize EsRelation we serialize the original EsIndex in addition to the `

nik9000 · 2024-08-06T12:44:28Z

When we serialize EsRelation we serialize the original EsIndex in addition to the `

Fields. To the fields.

@astefan check out the transport change I just pushed. It looks like we serialize the mapping of the index in addition to the FieldAttributes. I'm not sure we do anything with it. I'm tempted to remove it to be honest. But not here. The transport change I've added gets the job done and limits the blast radius of the unexpected serialization.

nik9000 · 2024-08-06T14:50:53Z

@astefan I pushed a change just now to explain the reading change better. It's to do with the way we reuse a function for serialization.

astefan · 2024-08-06T17:37:26Z

Thank you @nik9000

nik9000 · 2024-08-06T17:49:09Z

Thank you @nik9000

That sounds like and approal. I'm mash the merge button. Let me know if you have any more ideas around this one. It's been a trip!

* upstream/main: (132 commits) Fix compile after several merges Update docs with new behavior on skip conditions (elastic#111640) Skip on any instance of node or version features being present (elastic#111268) Skip on any node capability being present (elastic#111585) [DOCS] Publishes Anthropic inference service docs. (elastic#111619) Introduce `ChunkedZipResponse` (elastic#109820) [Gradle] fix esql compile cacheability (elastic#111651) Mute org.elasticsearch.datastreams.logsdb.qa.StandardVersusLogsIndexModeChallengeRestIT testTermsQuery elastic#111666 Mute org.elasticsearch.datastreams.logsdb.qa.StandardVersusLogsIndexModeChallengeRestIT testMatchAllQuery elastic#111664 Mute org.elasticsearch.xpack.esql.analysis.VerifierTests testMatchCommand elastic#111661 Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommandWithMultipleMatches {default} elastic#111660 Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommand {default} elastic#111659 Mute org.elasticsearch.xpack.esql.optimizer.LocalPhysicalPlanOptimizerTests testMatchCommandWithWhereClause {default} elastic#111658 LogsDB qa tests - add specific matcher for source (elastic#111568) ESQL: Move `randomLiteral` (elastic#111647) [ESQL] Clean up UNSUPPORTED type blocks (elastic#111648) ESQL: Remove the `NESTED` DataType (elastic#111495) ESQL: Move more out of esql-core (elastic#111604) Improve MvPSeriesWeightedSum edge case and add more tests (elastic#111552) Add link to flood-stage watermark exception message (elastic#111315) ... # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

We don't support nested fields at the moment and when we do I'm not sure it'll *be* a DataType. Maybe. Probably. But let's deal with that when we support it.

ESQL: Remove the NESTED DataType

581ac34

We don't support nested fields at the moment and when we do I'm not sure it'll *be* a DataType. Maybe. Probably. But let's deal with that when we support it.

nik9000 added >non-issue :Analytics/ES|QL AKA ESQL v8.16.0 labels Jul 31, 2024

nik9000 commented Jul 31, 2024

View reviewed changes

nik9000 requested a review from astefan July 31, 2024 20:15

nik9000 marked this pull request as ready for review July 31, 2024 20:15

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

nik9000 added 2 commits August 1, 2024 12:26

More tests for nested

d0405b2

Merge branch 'main' into esql_drop_nested

ff5b756

nik9000 added 2 commits August 1, 2024 13:03

Maybe this

4a94155

Test

2fa37c1

astefan approved these changes Aug 2, 2024

View reviewed changes

nik9000 added 2 commits August 5, 2024 12:40

Merge branch 'main' into esql_drop_nested

890be2d

more tests

47dac60

nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 5, 2024

nik9000 added 2 commits August 6, 2024 08:22

Merge branch 'main' into esql_drop_nested

deef905

Transport change.....

98c316d

nik9000 removed the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 6, 2024

Explain the read more

25460e9

Merge branch 'main' into esql_drop_nested

5a6cbbd

nik9000 merged commit 3d6b7aa into elastic:main Aug 6, 2024
15 checks passed

mhl-b pushed a commit that referenced this pull request Aug 8, 2024

ESQL: Remove the NESTED DataType (#111495)

47196db

We don't support nested fields at the moment and when we do I'm not sure it'll *be* a DataType. Maybe. Probably. But let's deal with that when we support it.

astefan mentioned this pull request Nov 20, 2024

ESQL: use field_caps native nested fields filtering #117201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ESQL: Remove the `NESTED` DataType #111495

ESQL: Remove the `NESTED` DataType #111495

nik9000 commented Jul 31, 2024

nik9000 Jul 31, 2024

nik9000 Jul 31, 2024

nik9000 Aug 1, 2024

astefan Aug 2, 2024

elasticsearchmachine commented Jul 31, 2024

nik9000 commented Aug 1, 2024

nik9000 commented Aug 1, 2024

nik9000 commented Aug 1, 2024

nik9000 commented Aug 1, 2024

astefan left a comment

astefan Aug 2, 2024

nik9000 commented Aug 2, 2024

nik9000 commented Aug 5, 2024

nik9000 commented Aug 6, 2024

nik9000 commented Aug 6, 2024

astefan commented Aug 6, 2024

nik9000 commented Aug 6, 2024

ESQL: Remove the NESTED DataType #111495

ESQL: Remove the NESTED DataType #111495

Conversation

nik9000 commented Jul 31, 2024

nik9000 Jul 31, 2024

Choose a reason for hiding this comment

nik9000 Jul 31, 2024

Choose a reason for hiding this comment

nik9000 Aug 1, 2024

Choose a reason for hiding this comment

astefan Aug 2, 2024

Choose a reason for hiding this comment

elasticsearchmachine commented Jul 31, 2024

nik9000 commented Aug 1, 2024

nik9000 commented Aug 1, 2024

nik9000 commented Aug 1, 2024

nik9000 commented Aug 1, 2024

astefan left a comment

Choose a reason for hiding this comment

astefan Aug 2, 2024

Choose a reason for hiding this comment

nik9000 commented Aug 2, 2024

nik9000 commented Aug 5, 2024

nik9000 commented Aug 6, 2024

nik9000 commented Aug 6, 2024

astefan commented Aug 6, 2024

nik9000 commented Aug 6, 2024

ESQL: Remove the `NESTED` DataType #111495

ESQL: Remove the `NESTED` DataType #111495