Skip to content

ESQL: Reuse Block Reader only when few fields #141672

Merged
nik9000 merged 8 commits intoelastic:mainfrom
nik9000:esql_supply_block_loader
Feb 3, 2026
Merged

ESQL: Reuse Block Reader only when few fields #141672
nik9000 merged 8 commits intoelastic:mainfrom
nik9000:esql_supply_block_loader

Conversation

@nik9000
Copy link
Copy Markdown
Member

@nik9000 nik9000 commented Feb 2, 2026

Stops ESQL from reusing the BlockLoad.ColumnAtATimeReaders when
loading many of these fields at once. Attempting to reuse these readers
means we have to keep all of them in memory. If we don't reuse we can
release the memory for each field as we load each Block's. When you
load hundreds of blocks this really adds up.

Important: This only works for column-at-a-time readers. This waits for a follow-up change. For truly row-by-row readers, this is fine. They don't use much memory anyway. But we use row-by-row readers as a fallback for reading doc values when loading from many segments. That seems important. Usually if the query wants to load hundreds of fields, it's after a topn. And usually those are "from many segments".

Stops ESQL from reusing the `BlockLoad.ColumnAtATimeReader`s when
loading many of these fields at once. Attempting to reuse these readers
means we have to keep all of them in memory. If we don't reuse we can
release the memory for each field as we load each `Block`'s. When you
load hundreds of blocks this really adds up.

Important: This only works for column-at-a-time readers. Mostly, the
row-by-row readers don't take much space anyway.
@nik9000 nik9000 requested a review from martijnvg February 2, 2026 17:01
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 2, 2026
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Feb 2, 2026

I'm still looking into heap attack tests. I might need the follow-up work to really hit - but chekcing.

Copy link
Copy Markdown
Member Author

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. A few heap attack tests pass now.

Next I'll see if I can get ColumnAtATimeReader working the FromMany reader.

Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Nik! I think this looks good.

Looks like some unit test failures for test expecting block loader but now get io supplier. This can be fixed by resolving the supplier and then checking to block loader.

s.storedFieldsSequentialProportion()
)
);
boolean reuseColumnLoaders = fieldExtractExec.attributesToExtract().size() <= context.plannerSettings()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This includes fields used for sorting, grouping, sorting etc? I think if sorting does gets pushed down, then those fields aren't part of attributes to extract. Similar to when WHERE gets pushed down. In the push down case, we aren't being slowed down here.

Maybe for other things we could have exceptions. Like for TS command for _tsid and @timestamp. Not in this PR and maybe in a follow up.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used for almost all loading ESQL has to do - sorts, groups, aggs, returning the column. All of it.

I'm not sure we really need exceptions though. This'll only come up if you need more than 30 fields at a time. Mostly this'll come up for stuff like FROM foo | LIMIT 10. Aggs rarely touch 30 fields at a time. But you can make them do it - testAggManyFieldsNoReuse does that. But it's kind of a lot.

If you use time series stuff to get the last value of like 50 fields. Or the rate of that many fields. Then this'll kick in. Probably. I don't know all of the bits y'all have.

* the paths that need very high performance don't load more than a handful of fields at a time,
* so they <strong>do</strong> reuse fields.
*/
public static final Setting<Integer> REUSE_COLUMN_LOADERS_THRESHOLD = Setting.intSetting(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this statement.

@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Feb 3, 2026

Looks like some unit test failures for test expecting block loader but now get io supplier. This can be fixed by resolving the supplier and then checking to block loader.

👍

@nik9000 nik9000 merged commit 4133069 into elastic:main Feb 3, 2026
36 checks passed
jordan-powers added a commit to jordan-powers/elasticsearch that referenced this pull request Feb 3, 2026
nik9000 added a commit that referenced this pull request Feb 3, 2026
We were growing more and more and more options to
`OperatorTestCase.runDriver`. I need another option in #141672 so I built
a builder-style test utility. This removes the original methods, migrating
all callers.

I've been quite liberal adding utility methods. Those are cheap in a
builder-style helper because you don't have to think in terms of
combinatorial explosions of parameter - just in terms of "how do I set
up all the bits". Now there are ten ways to set the inputs.

It's tempting to make some higher level utility methods that call these -
or make the common call sites shorter. You init the most common helper like:
```
new TestDriverRunner().builder(driverContext())
```
But I didn't want it to look like magic. Readers should see this and
think, "I can add things before `builder`" and "I can add things to
this `builder`."
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Feb 4, 2026
Stops ESQL from reusing the `BlockLoad.ColumnAtATimeReader`s when
loading many of these fields at once. Attempting to reuse these readers
means we have to keep all of them in memory. If we don't reuse we can
release the memory for each field as we load each `Block`'s. When you
load hundreds of blocks this really adds up.

Important: This only works for column-at-a-time readers. Mostly, the
row-by-row readers don't take much space anyway.
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Feb 4, 2026
We were growing more and more and more options to
`OperatorTestCase.runDriver`. I need another option in elastic#141672 so I built
a builder-style test utility. This removes the original methods, migrating
all callers.

I've been quite liberal adding utility methods. Those are cheap in a
builder-style helper because you don't have to think in terms of
combinatorial explosions of parameter - just in terms of "how do I set
up all the bits". Now there are ten ways to set the inputs.

It's tempting to make some higher level utility methods that call these -
or make the common call sites shorter. You init the most common helper like:
```
new TestDriverRunner().builder(driverContext())
```
But I didn't want it to look like magic. Readers should see this and
think, "I can add things before `builder`" and "I can add things to
this `builder`."
spinscale pushed a commit to spinscale/elasticsearch that referenced this pull request Feb 4, 2026
Stops ESQL from reusing the `BlockLoad.ColumnAtATimeReader`s when
loading many of these fields at once. Attempting to reuse these readers
means we have to keep all of them in memory. If we don't reuse we can
release the memory for each field as we load each `Block`'s. When you
load hundreds of blocks this really adds up.

Important: This only works for column-at-a-time readers. Mostly, the
row-by-row readers don't take much space anyway.
spinscale pushed a commit to spinscale/elasticsearch that referenced this pull request Feb 4, 2026
We were growing more and more and more options to
`OperatorTestCase.runDriver`. I need another option in elastic#141672 so I built
a builder-style test utility. This removes the original methods, migrating
all callers.

I've been quite liberal adding utility methods. Those are cheap in a
builder-style helper because you don't have to think in terms of
combinatorial explosions of parameter - just in terms of "how do I set
up all the bits". Now there are ten ways to set the inputs.

It's tempting to make some higher level utility methods that call these -
or make the common call sites shorter. You init the most common helper like:
```
new TestDriverRunner().builder(driverContext())
```
But I didn't want it to look like magic. Readers should see this and
think, "I can add things before `builder`" and "I can add things to
this `builder`."
mamazzol pushed a commit to mamazzol/elasticsearch that referenced this pull request Feb 5, 2026
We were growing more and more and more options to
`OperatorTestCase.runDriver`. I need another option in elastic#141672 so I built
a builder-style test utility. This removes the original methods, migrating
all callers.

I've been quite liberal adding utility methods. Those are cheap in a
builder-style helper because you don't have to think in terms of
combinatorial explosions of parameter - just in terms of "how do I set
up all the bits". Now there are ten ways to set the inputs.

It's tempting to make some higher level utility methods that call these -
or make the common call sites shorter. You init the most common helper like:
```
new TestDriverRunner().builder(driverContext())
```
But I didn't want it to look like magic. Readers should see this and
think, "I can add things before `builder`" and "I can add things to
this `builder`."
nik9000 added a commit that referenced this pull request Feb 10, 2026
ESQL: Load many fields column-at-a-time

Adds support for `ColumnAtATimeReader` in the case where we're loading
from many segments. This should marginally speed up loading many
documents after a top n. More importantly, it lets #141672 kick in
when loading from many fields. This should save significantly memory
when loading thousands of fields after a `| SORT | LIMIT` sequence.

Finally, this changes the rules for `BlockLoader`. Previously you
*could* return `null` from `columnAtATimeReader` but must never return
`null` from `rowStrideReader`. Now the rule is that you may return null
from *either* of the two, but not both. This should let us delete a
bunch of code. While we're at it, we should add a
`read(builder, docs, offset, nullsFiltered)` override to save a copy.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL >bug Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.4.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants