Do not serialize EsIndex in plan #119580

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

@@ -2644,7 +2644,6 @@ private void assertEmptyEsRelation(LogicalPlan plan) {
        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?

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

        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";

}

costin · 2025-01-08T06:24:28Z

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

-    private final EsIndex index;
+    private final String indexName;
+    private final IndexMode indexMode;
+    private final Map<String, IndexMode> indexNameWithModes;
    private final List<Attribute> attrs;
    private final boolean frozen;


frozen is no longer needed - since you're refactoring the classes, please remove this field from the Es classes.

# 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

@@ -126,12 +129,13 @@ public void testDeeplyNestedFields() throws IOException {
     * 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%!

alex-spies · 2025-01-09T15:50:18Z

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

+    public static final TransportVersion ESQL_SKIP_ES_INDEX_SERIALIZATION = def(8_823_00_0);
+    public static final TransportVersion ESQL_REMOVE_ES_RELATION_FROZEN = def(8_824_00_0);


nit: couldn't that be a single new transport version?

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

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

        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!

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

fix test

a09ddc0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not serialize EsIndex in plan #119580

Do not serialize EsIndex in plan #119580

idegtiarenko commented Jan 6, 2025

elasticsearchmachine commented Jan 6, 2025

idegtiarenko Jan 6, 2025

elasticsearchmachine commented Jan 6, 2025

costin left a comment

costin Jan 8, 2025

idegtiarenko Jan 9, 2025

alex-spies Jan 9, 2025

alex-spies Jan 10, 2025

costin Jan 8, 2025

alex-spies Jan 7, 2025

alex-spies Jan 9, 2025

alex-spies Jan 9, 2025

		IndexMode indexMode,
		Map<String, IndexMode> indexNameWithModes,

	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";
	}

		public static final TransportVersion ESQL_SKIP_ES_INDEX_SERIALIZATION = def(8_823_00_0);
		public static final TransportVersion ESQL_REMOVE_ES_RELATION_FROZEN = def(8_824_00_0);

Do not serialize EsIndex in plan #119580

Are you sure you want to change the base?

Do not serialize EsIndex in plan #119580

Conversation

idegtiarenko commented Jan 6, 2025

elasticsearchmachine commented Jan 6, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment