Do not serialize EsIndex in plan by idegtiarenko · Pull Request #119580 · elastic/elasticsearch

idegtiarenko · 2025-01-06T13:09:42Z

Certain plan classes (such as EsRelation, EsSourceExec, EsQueryExec) contain and serialize the entire EsIndex instance.
This instance might contain huge mapping that is never used in plan. This change replaces EsIndex usage with name and indexNameWithModes to minimize the size of the serialized plan.

Closes: #112998

elasticsearchmachine · 2025-01-06T13:10:13Z

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

idegtiarenko · 2025-01-06T13:12:04Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java

        assertThat(plan, instanceOf(EsRelation.class));
        EsRelation esRelation = (EsRelation) plan;
        assertThat(esRelation.output(), equalTo(NO_FIELDS));
-        assertTrue(esRelation.index().mapping().isEmpty());


Not sure if it is possible to replace this assertion in assertEmptyEsRelation with any alternative?

I think the assertion on output above is sufficient. Asserting the empty mapping here looks like this was over constraining, I think.

elasticsearchmachine · 2025-01-06T15:10:02Z

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

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/EsRelation.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsSourceExec.java

costin

Great find - left a question around the usage of IndexModes map; not clear why that has to be serialized?

costin · 2025-01-08T06:23:44Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java

-        EsIndex index,
+        String indexName,
        IndexMode indexMode,
+        Map<String, IndexMode> indexNameWithModes,


Why is this map needed?

It is used in LocalExecutionPlanner:

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

Lines 564 to 571 in 2ff1d76

Map<String, IndexMode> indicesWithModes = localSourceExec.indexNameWithModes();

if (indicesWithModes.size() != 1) {

throw new IllegalArgumentException("can't plan [" + join + "], found more than 1 index");

}

var entry = indicesWithModes.entrySet().iterator().next();

if (entry.getValue() != IndexMode.LOOKUP) {

throw new IllegalArgumentException("can't plan [" + join + "], found index with mode [" + entry.getValue() + "]");

}

If this is the only thing that requires that: I believe this check could happen much earlier. Potentially already in the analyzer on the coordinator node. We shouldn't need to wait until the local planning to determine that an index isn't a lookup index.

Could you maybe check if it's feasible to move that to an earlier place and, thus, remove the indexNameWithModes map altogether from the EsRelation and related classes?

Happy to assist with moving that to an earlier stage!

There already is an index mode check in the Analyzer:

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

Lines 239 to 255 in 4ac8d55

if (plan.indexMode().equals(IndexMode.LOOKUP)) {

String indexResolutionMessage = null;

var indexNameWithModes = esIndex.indexNameWithModes();

if (indexNameWithModes.size() != 1) {

indexResolutionMessage = "invalid ["

+ table

+ "] resolution in lookup mode to ["

+ indexNameWithModes.size()

+ "] indices";

} else if (indexNameWithModes.values().iterator().next() != IndexMode.LOOKUP) {

indexResolutionMessage = "invalid ["

+ table

+ "] resolution in lookup mode to an index in ["

+ indexNameWithModes.values().iterator().next()

+ "] mode";

}

I think I got it. The check is indeed already performed and could be removed - it's just there out of defensiveness as far as I can see.

What cannot be removed is the line

var entry = indicesWithModes.entrySet().iterator().next();

which, in case of aliases, contains the actual lookup index name in the entry's key.

Looks to me like we should add a comment and leave this as-is. (Well, and maybe remove the defensive double validation, but it also doesn't hurt.)

I'll confirm with @idegtiarenko if this is what we want to do.

Correct. IndexName could actually be alias or pattern, while map entries include resolved index names

@costin , we decided removing this map is too large in scope for this PR. Ievgen opened a follow-up issue: #120210

The map seems similar to EsIndex - it's detailed information that isn't required.
Nit: doing it in a separate PR will require a transport bump; maybe consider this complete but not merge, do the follow-up PR and then merge them at the same time to have only one increment?

I am not sure I would be able to pick up followup immediately.
Also keeping this updated required multiple conflict resolutions.
I would really prefer to merge this earlier.

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/EsRelation.java

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

alex-spies · 2025-01-07T14:17:54Z

...test/java/org/elasticsearch/xpack/esql/plan/physical/ExchangeSinkExecSerializationTests.java

     * with a single root field that has many children, grandchildren etc.
     */
    public void testDeeplyNestedFieldsKeepOnlyOne() throws IOException {
-        ByteSizeValue expected = ByteSizeValue.ofBytes(9425804);


This test file beautifully demonstrates the improvements from the change. Awesome, in this particularly bad case we got a reduction by 99.996%!

server/src/main/java/org/elasticsearch/TransportVersions.java

alex-spies · 2025-01-09T16:02:08Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java

-        EsIndex index,
+        String indexName,
        IndexMode indexMode,
+        Map<String, IndexMode> indexNameWithModes,


If this is the only thing that requires that: I believe this check could happen much earlier. Potentially already in the analyzer on the coordinator node. We shouldn't need to wait until the local planning to determine that an index isn't a lookup index.

Could you maybe check if it's feasible to move that to an earlier place and, thus, remove the indexNameWithModes map altogether from the EsRelation and related classes?

Happy to assist with moving that to an earlier stage!

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

alex-spies · 2025-01-15T11:32:26Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/EsRelation.java

-    public EsRelation(Source source, EsIndex index, List<Attribute> attributes, IndexMode indexMode, boolean frozen) {
+    public EsRelation(
+        Source source,
+        String indexName,


In many cases, there will not be a concrete index name, but an index pattern. I think it's slightly misleading to call this indexName for this reason, as it gives the impression as if this was one singular index. E.g. FROM logs* will have logs* as the index name, but actually that's not the name of any index.

Suggested change

String indexName,

String indexPattern,

alex-spies · 2025-01-15T11:36:54Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java

-        EsIndex index,
+        String indexName,
        IndexMode indexMode,
+        Map<String, IndexMode> indexNameWithModes,


I think I got it. The check is indeed already performed and could be removed - it's just there out of defensiveness as far as I can see.

What cannot be removed is the line

var entry = indicesWithModes.entrySet().iterator().next();

which, in case of aliases, contains the actual lookup index name in the entry's key.

Looks to me like we should add a comment and leave this as-is. (Well, and maybe remove the defensive double validation, but it also doesn't hurt.)

I'll confirm with @idegtiarenko if this is what we want to do.

alex-spies · 2025-01-15T11:52:26Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/EsRelation.java

+    public EsRelation(
+        Source source,
+        String indexName,
+        IndexMode indexMode,


I believe the indexMode attribute has become obsolete. It's required in the UnresolvedRelation, but if we keep track of indexNameWithModes, the latter has more complete information obtained from actual field caps calls and should be preferred.

I think we only use this indexMode to check if a relation corresponds to a lookup index. I think we could just have a helper function instead isLookupIndex which checks the indexNameWithModes.

Let me try to replace its usages with the map

We also have 2 checks for TIME_SERIES:

elasticsearch/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/physical/local/ReplaceSourceAttributes.java

Line 35 in ab26077

if (plan.indexMode() == IndexMode.TIME_SERIES) {

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

Line 184 in d953079

if (esQueryExec.indexMode() == IndexMode.TIME_SERIES) {

that is originally configured in:

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

Line 484 in 212e16b

IndexMode.TIME_SERIES,

I am thinking of deferring that to a separate change

I opened an issue for that #120210

This reverts commit 3685545

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

costin

LGTM

costin · 2025-01-17T00:43:48Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsQueryExec.java

-        EsIndex index,
+        String indexName,
        IndexMode indexMode,
+        Map<String, IndexMode> indexNameWithModes,


The map seems similar to EsIndex - it's detailed information that isn't required.
Nit: doing it in a separate PR will require a transport bump; maybe consider this complete but not merge, do the follow-up PR and then merge them at the same time to have only one increment?

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java

alex-spies · 2025-01-17T09:03:18Z

@costin:

The map seems similar to EsIndex - it's detailed information that isn't required.
Nit: doing it in a separate PR will require a transport bump; maybe consider this complete but not merge, do the follow-up PR and then merge them at the same time to have only one increment?

@idegtiarenko and I went over this, and the map is not redundant; compared to EsIndex.indexMode, this contains data retrieved from the field caps response and is used in some places where we need to know the concrete indices that this EsIndex instance represents. And the actual (not presumed) index modes of said indices.

Removing this, or at least reducing it to the absolutely required minimum, is desired but more effort (quite a bit of disentangling different usages there); and we'd like to postpone this effort. The present PR delivers on the goal of reducing the redundantly serialized data greatly in many cases.

So I'd prefer getting this PR in, live with the additional transport version bump, and enjoy the benefit of leaner serialization already.

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumns.java

alex-spies

Thanks a ton, @idegtiarenko ! I think this is a great improvement.

I have only very minor comments; this can be merged as-is, please treat the comments as optional.

When backporting, I think it's important we run the PR with test-full-bwc to make sure that we don't accidentally break bwc with 8.17, 8.16 etc. On the off chance that we break bwc with e.g. 8.15, we might need to skip a test or so based on a capability.
To ensure this, maybe it's better to remove the auto-backport label and do the backport manually.

alex-spies · 2025-01-20T09:29:54Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsSourceExec.java

    }

-    public EsSourceExec(Source source, EsIndex index, List<Attribute> attributes, QueryBuilder query, IndexMode indexMode) {
+    public EsSourceExec(


super nit: we could make the constructor's parameter order here consistent with that of EsQueryExec's, in that the attributes come before the query.

alex-spies · 2025-01-20T09:35:26Z

...k/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/physical/EsStatsQueryExec.java

    public EsStatsQueryExec(
        Source source,
-        EsIndex index,
+        String indexName,


nit: consistency

Suggested change

String indexName,

String indexPattern,

If you decide to align this, let's not forget about the nodeString as well.

alex-spies · 2025-01-20T09:40:19Z

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java

        assertThat(plan, instanceOf(EsRelation.class));
        EsRelation esRelation = (EsRelation) plan;
        assertThat(esRelation.output(), equalTo(NO_FIELDS));
-        assertTrue(esRelation.index().mapping().isEmpty());


I think the assertion on output above is sufficient. Asserting the empty mapping here looks like this was over constraining, I think.

alex-spies · 2025-01-20T12:04:10Z

...test/java/org/elasticsearch/xpack/esql/plan/physical/ExchangeSinkExecSerializationTests.java

+    /**
+     * Test the size of serializing a plan like
+     * FROM index* | LIMIT 10 | KEEP one_single_field
+     * with an index pattern pointing to a hundred actual indices with rather long names
+     */
+    public void testIndexPatternTargetingMultipleIndices() throws IOException {


++, thank you for the test case!

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

elasticsearchmachine · 2025-01-21T08:14:45Z

💚 Backport successful

Status	Branch	Result
✅	8.x

Certain plan classes (such as EsRelation, EsSourceExec, EsQueryExec) contain and serialize the entire EsIndex instance. This instance might contain huge mapping that is never used in plan. This change replaces EsIndex usage with indexPattern and indexNameWithModes to minimize the size of the serialized plan.

idegtiarenko added 6 commits January 6, 2025 13:15

EsQueryExec

2ff1d76

EsStatsQueryExec

e3fe769

cleanup serialization

ce0fbde

EsSourceExec

b6c17b8

EsRelation

cb41a7a

format

c283fd3

idegtiarenko added >enhancement Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.0.0 v8.18.0 labels Jan 6, 2025

idegtiarenko requested a review from alex-spies January 6, 2025 13:09

Update docs/changelog/119580.yaml

d45ed5a

idegtiarenko commented Jan 6, 2025

View reviewed changes

fix sizing tests

cfc22d9

idegtiarenko marked this pull request as ready for review January 6, 2025 15:09

costin reviewed Jan 8, 2025

View reviewed changes

idegtiarenko added 3 commits January 9, 2025 14:58

Merge branch 'main' into do_not_serialize_es_index

d1effd7

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

fix typo

93c93da

remove frozen flag

bdfa4f3

alex-spies reviewed Jan 9, 2025

View reviewed changes

alex-spies self-requested a review January 9, 2025 16:31

idegtiarenko added 4 commits January 10, 2025 08:12

fix test

a09ddc0

Merge branch 'main' into do_not_serialize_es_index

13437ca

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

remove unused maps

3685545

Merge branch 'main' into do_not_serialize_es_index

60548d3

alex-spies reviewed Jan 15, 2025

View reviewed changes

idegtiarenko added 4 commits January 15, 2025 14:19

Revert "remove unused maps"

ab26077

This reverts commit 3685545

Merge branch 'main' into do_not_serialize_es_index

179c0a4

rename field

b1e959b

add a test case when pointing multiple indices

fa92d65

idegtiarenko requested review from alex-spies and costin January 15, 2025 15:24

Merge branch 'main' into do_not_serialize_es_index

b408a30

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

costin approved these changes Jan 17, 2025

View reviewed changes

Merge branch 'main' into do_not_serialize_es_index

6d16184

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/analysis/AnalyzerTests.java

idegtiarenko added 2 commits January 17, 2025 10:31

fix pattern

7a7f309

Merge branch 'main' into do_not_serialize_es_index

abe15c6

# Conflicts: # x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/optimizer/rules/logical/PruneColumns.java

alex-spies approved these changes Jan 20, 2025

View reviewed changes

idegtiarenko added test-full-bwc Trigger full BWC version matrix tests v8.16.0 and removed auto-backport Automatically create backport pull requests when merged v8.16.0 v8.18.0 labels Jan 20, 2025

align parameters order

d63db73

luigidellaquila mentioned this pull request Jan 20, 2025

ES|QL: quantify memory footprint (live objects and serialization) of query plan #111358

Open

8 tasks

Merge branch 'main' into do_not_serialize_es_index

c86eb30

# Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java

idegtiarenko added auto-backport Automatically create backport pull requests when merged v8.18.0 labels Jan 21, 2025

idegtiarenko merged commit 5ab81a9 into elastic:main Jan 21, 2025
16 checks passed

idegtiarenko mentioned this pull request Jan 21, 2025

[8.x] Do not serialize EsIndex in plan (#119580) #120496

Merged

idegtiarenko deleted the do_not_serialize_es_index branch January 21, 2025 08:16

	Map<String, IndexMode> indicesWithModes = localSourceExec.indexNameWithModes();
	if (indicesWithModes.size() != 1) {
	throw new IllegalArgumentException("can't plan [" + join + "], found more than 1 index");
	}
	var entry = indicesWithModes.entrySet().iterator().next();
	if (entry.getValue() != IndexMode.LOOKUP) {
	throw new IllegalArgumentException("can't plan [" + join + "], found index with mode [" + entry.getValue() + "]");
	}

	if (plan.indexMode().equals(IndexMode.LOOKUP)) {
	String indexResolutionMessage = null;

	var indexNameWithModes = esIndex.indexNameWithModes();
	if (indexNameWithModes.size() != 1) {
	indexResolutionMessage = "invalid ["
	+ table
	+ "] resolution in lookup mode to ["
	+ indexNameWithModes.size()
	+ "] indices";
	} else if (indexNameWithModes.values().iterator().next() != IndexMode.LOOKUP) {
	indexResolutionMessage = "invalid ["
	+ table
	+ "] resolution in lookup mode to an index in ["
	+ indexNameWithModes.values().iterator().next()
	+ "] mode";
	}

Conversation

idegtiarenko commented Jan 6, 2025

elasticsearchmachine commented Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 6, 2025

costin 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

alex-spies Jan 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idegtiarenko Jan 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-spies commented Jan 17, 2025

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

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jan 21, 2025

💚 Backport successful

Labels

4 participants

alex-spies Jan 16, 2025 •

edited

Loading

idegtiarenko Jan 17, 2025 •

edited

Loading