ESQL: Fix double release in inline stats when `LocalRelation` is reused #136467

kanoshiou · 2025-10-13T08:33:18Z

When the optimizer creates a LocalRelation using LocalSupplier.of() (which delegates to ImmediateLocalSupplier), the same page instance can be referenced and released multiple times during inline stats execution. This happens because the LocalRelation serves as a source for both sides of the InlineJoin - once for the left-hand side and again for the right-hand side, causing a double release error and connection closure.

This PR modifies the InlineJoin.firstSubPlan() method to ensure that all LocalRelation nodes use CopyingLocalSupplier (if they don't already) after extracting the subplan. This prevents double release issues when the same page is reused across executions.

Closes #135679

- Replace `LocalSupplier.of` with new `CopyingLocalSupplier` in `PruneColumns` - Add `INLINE_STATS_AFTER_PRUNED_AGGREGATE` capability - Add test cases for inline stats after aggregate pruning

elasticsearchmachine · 2025-10-13T10:36:47Z

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

bpintea · 2025-10-14T17:20:51Z

@kanoshiou thanks for this patch!

I'm however thinking if we shouldn't have another approach here: instead of patching the optimiser rules to use the CopyingLocalSupplier, shouldn't we rather patch theLocalRelation leaf that InlineJoin get to run on the RHS? (Possibly apply the fix only to InlineJoin#firstSubPlan.)
#135679 is INLINE STATS-specific and we could probably contain the fix to it. We could then also check if we'd need a CopyingLocalSupplier (but that's secondary).

@astefan, WDYT?

astefan · 2025-10-15T16:07:58Z

@kanoshiou thanks for this patch!

I'm however thinking if we shouldn't have another approach here: instead of patching the optimiser rules to use the CopyingLocalSupplier, shouldn't we rather patch theLocalRelation leaf that InlineJoin get to run on the RHS? (Possibly apply the fix only to InlineJoin#firstSubPlan.) #135679 is INLINE STATS-specific and we could probably contain the fix to it. We could then also check if we'd need a CopyingLocalSupplier (but that's secondary).

@astefan, WDYT?

@kanoshiou inline stats is different than any other command in that part of its workings live in EsqlSession. Whenever we add something that is specific to inline stats we try to limit its spread and make it as seamless as possible in the existing code.
This means that modifications that happen outside EsqlSession and InlineJoin itself should be limited. Your changes to optimizer rules that have nothing to do with inline stats (as opposed to PruneInlineJoinOnEmptyRightSide for example which is inline stats specific) should have a very good reason to be there.

Your changes to PruneColumns and PropagateEmptyRelation take the shotgun approach and are too invasive, they will also apply to non-inline stats use cases which is unnecessary. @bpintea's suggestion is something you should look into, instead.

astefan

Please, look into @bpintea's suggestion for a less invasive and more targeted approach to this bug fix. Please, let us know if you are able to work on this bug fix in short time, otherwise we are ready to take it over. Thank you.

kanoshiou · 2025-10-15T17:34:32Z

Thank you for the review and insightful suggestions, @bpintea @astefan! I'll update the branch today with a new logical optimizer rule.

astefan

I had something else in mind when I did my first review.
Using a CopyingLocalSupplier should happen in InlineJoin.firstSubPlan:

public static LogicalPlanTuple firstSubPlan(LogicalPlan optimizedPlan, Set<LocalRelation> subPlansResults) {
        final Holder<LogicalPlan> stubReplacedSubPlanHolder = new Holder<>();
        final Holder<LogicalPlan> originalSubPlanHolder = new Holder<>();
        // Collect the first inlinejoin (bottom up in the tree)
        optimizedPlan.forEachUp(InlineJoin.class, ij -> {
            // extract the right side of the plan and replace its source
            if (stubReplacedSubPlanHolder.get() == null) {
                if (ij.right().anyMatch(p -> p instanceof StubRelation)) {
                    stubReplacedSubPlanHolder.set(replaceStub(ij.left(), ij.right()));
                } else if (ij.right() instanceof LocalRelation relation
                    && (subPlansResults.isEmpty() || subPlansResults.contains(relation) == false)
                    || ij.right() instanceof LocalRelation == false && ij.right().anyMatch(p -> p instanceof LocalRelation)) {
                    // In case the plan was optimized further and the StubRelation was replaced with a LocalRelation
                    // or the right hand side became a LocalRelation alltogether, there is no need to replace the source of the
                    // right-hand side anymore.
                    stubReplacedSubPlanHolder.set(ij.right());
                    // TODO: INLINE STATS this is essentially an optimization similar to the one in PruneInlineJoinOnEmptyRightSide
                    // this further supports the idea of running the optimization step again after the substitutions (see EsqlSession
                    // executeSubPlan() method where we could run the optimizer after the results are replaced in place).
                }
                originalSubPlanHolder.set(ij.right());
            }
        });
        LogicalPlanTuple tuple = null;
        if (stubReplacedSubPlanHolder.get() != null) {
            var plan = stubReplacedSubPlanHolder.get();
            plan = plan.transformUp(LocalRelation.class, lr -> {
                if (lr.supplier() instanceof CopyingLocalSupplier == false) {
                    return new LocalRelation(lr.source(), lr.output(), new CopyingLocalSupplier(lr.supplier().get()));
                }
                return lr;
            });
            plan.setOptimized();
            tuple = new LogicalPlanTuple(plan, originalSubPlanHolder.get());
        }
        return tuple;
    }

kanoshiou · 2025-10-16T13:14:02Z

Thank you for the thoughtful patch, @astefan — it really helped clarify the underlying issue!

It looks like your current approach addresses the problem outside the logical optimization phase, which seems like the right direction for tackling the root cause.

While reviewing your code, I was wondering if the CopyingLocalSupplier currently used in ReplaceRowAsLocalRelation could be replaced with an ImmediateLocalSupplier. Doing so might simplify the handling and reduce unnecessary duplication, assuming the reuse semantics remain intact.

astefan · 2025-10-16T13:24:03Z

Thank you for the thoughtful patch, @astefan — it really helped clarify the underlying issue!

It looks like your current approach addresses the problem outside the logical optimization phase, which seems like the right direction for tackling the root cause.

While reviewing your code, I was wondering if the CopyingLocalSupplier currently used in ReplaceRowAsLocalRelation could be replaced with an ImmediateLocalSupplier. Doing so might simplify the handling and reduce unnecessary duplication, assuming the reuse semantics remain intact.

I don't know. It needs to go through a round of testing to double check this.

astefan · 2025-10-16T14:28:54Z

@elasticmachine test this please

astefan · 2025-10-17T08:57:07Z

@elasticmachine test this please

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/action/EsqlCapabilities.java

x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec

…fix-inline-stats-pruned-aggregates

astefan

LGTM. CI is happy with the change in ReplaceRowAsLocalRelation, I am happy as well.

astefan · 2025-10-17T12:45:27Z

@elasticmachine test this please

kanoshiou · 2025-10-17T14:00:27Z

Thanks for your review and sorry for the delay in replying to your comment and updating the code, @astefan! I learned a lot about ES|QL and its optimization nuances from your code and advice! Thanks again for your time and expertise!

bpintea

Thanks, LGTM!

bpintea · 2025-10-17T17:03:13Z

x-pack/plugin/esql/qa/testFixtures/src/main/resources/inlinestats.csv-spec

+| stats PRezTcny = median(network.cost), `network.cost` = count(network.cost) 
+| drop network.cost 
+| inline stats QaoRGYyf = count(*) 
+| drop `PRezTcny`


These queries are generated. This one in particular is relatively clean, but those below are randomised and those identifiers are eye pain (most `` quotations aren't needed either). Usually, they can also be shrunk down to a "minimal" query that reproduces the issue.
Ideally, we'd minimise the queries and use "human-friendly" identifiers, but at a minimum, maybe indicate their source by adding a comment with the originating issue.

bpintea · 2025-10-17T17:06:39Z

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

+        if (stubReplacedSubPlanHolder.get() != null) {
+            var plan = stubReplacedSubPlanHolder.get();


Minor:

Suggested change

if (stubReplacedSubPlanHolder.get() != null) {

var plan = stubReplacedSubPlanHolder.get();

var plan = stubReplacedSubPlanHolder.get();

if (plan != null) {

bpintea · 2025-10-17T17:19:46Z

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

     * And uses the left-hand side's source as its source:
     * Aggregate[[],[MAX(x{r}#99,true[BOOLEAN]) AS y#102]]
     * \_Limit[1000[INTEGER],false]
     *   \_LocalRelation[[x{r}#99],[IntVectorBlock[vector=ConstantIntVector[positions=1, value=1]]]]


This javadoc might require an update to be inline with the added paragraph, but this is minor, can remain as is.

…fix-inline-stats-pruned-aggregates

astefan · 2025-10-17T20:37:22Z

@elasticmachine test this please

elasticsearchmachine · 2025-10-17T21:44:25Z

💔 Backport failed

Status	Branch	Result
❌	9.2	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 136467

…ed (elastic#136467) When the optimizer creates a `LocalRelation` using `LocalSupplier.of()` (which delegates to `ImmediateLocalSupplier`), the same page instance can be referenced and released multiple times during inline stats execution. This happens because the `LocalRelation` serves as a source for both sides of the `InlineJoin` - once for the left-hand side and again for the right-hand side, causing a double release error and connection closure. This PR modifies the `InlineJoin.firstSubPlan()` method to ensure that all `LocalRelation` nodes use `CopyingLocalSupplier` (if they don't already) after extracting the subplan. This prevents double release issues when the same page is reused across executions. Closes elastic#135679 (cherry picked from commit 968b623)

…#136848) Backports #136467

…ed (elastic#136467) When the optimizer creates a `LocalRelation` using `LocalSupplier.of()` (which delegates to `ImmediateLocalSupplier`), the same page instance can be referenced and released multiple times during inline stats execution. This happens because the `LocalRelation` serves as a source for both sides of the `InlineJoin` - once for the left-hand side and again for the right-hand side, causing a double release error and connection closure. This PR modifies the `InlineJoin.firstSubPlan()` method to ensure that all `LocalRelation` nodes use `CopyingLocalSupplier` (if they don't already) after extracting the subplan. This prevents double release issues when the same page is reused across executions. Closes elastic#135679

Fix inline stats after pruned aggregate by using CopyingLocalSupplier

a8f9a4d

- Replace `LocalSupplier.of` with new `CopyingLocalSupplier` in `PruneColumns` - Add `INLINE_STATS_AFTER_PRUNED_AGGREGATE` capability - Add test cases for inline stats after aggregate pruning

elasticsearchmachine added needs:triage Requires assignment of a team area label v9.3.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Oct 13, 2025

kanoshiou added 2 commits October 13, 2025 16:35

Update docs/changelog/136467.yaml

c989c71

Merge branch 'refs/heads/main' into fix-inline-stats-pruned-aggregates

d28c47c

gareth-ellis added the :Analytics/ES|QL AKA ESQL label Oct 13, 2025

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 13, 2025

elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Oct 13, 2025

bpintea requested review from astefan and bpintea October 13, 2025 11:23

kanoshiou added 2 commits October 14, 2025 13:38

Fix other scenarios

97a5900

Merge branch 'refs/heads/main' into fix-inline-stats-pruned-aggregates

906435a

kanoshiou changed the title ~~ESQL: Fix connection closed in inline stats after pruned aggregates~~ ESQL: Fix double release in inline stats when LocalRelation is reused Oct 14, 2025

kanoshiou added 3 commits October 14, 2025 13:55

Update docs/changelog/136467.yaml

29c545e

Update tests

58345d1

Update tests

31bcc3b

astefan requested changes Oct 15, 2025

View reviewed changes

Merge branch 'refs/heads/main' into fix-inline-stats-pruned-aggregates

40f93f9

kanoshiou added 2 commits October 16, 2025 01:38

Optimize inline join and local relation handling

712a32b

Merge branch 'refs/heads/main' into fix-inline-stats-pruned-aggregates

b045514

kanoshiou requested a review from astefan October 15, 2025 17:42

astefan requested changes Oct 16, 2025

View reviewed changes

Using astefan's approach

4cc2558

astefan added >bug v9.2.1 auto-backport Automatically create backport pull requests when merged labels Oct 16, 2025

astefan self-assigned this Oct 16, 2025

kanoshiou added 2 commits October 17, 2025 12:27

required_capability: fork_v9

7259a5b

Merge branch 'refs/heads/main' into fix-inline-stats-pruned-aggregates

ee62799

kanoshiou requested a review from astefan October 17, 2025 04:28

astefan reviewed Oct 17, 2025

View reviewed changes

astefan added 2 commits October 17, 2025 15:30

capabilities clean up

85f1808

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

ae47b5c

…fix-inline-stats-pruned-aggregates

astefan approved these changes Oct 17, 2025

View reviewed changes

Move capability

e5444f3

astefan force-pushed the fix-inline-stats-pruned-aggregates branch from 9eb00fc to e5444f3 Compare October 17, 2025 12:44

bpintea approved these changes Oct 17, 2025

View reviewed changes

astefan added 2 commits October 17, 2025 23:33

Address reviews

a52977e

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

5ca4666

…fix-inline-stats-pruned-aggregates

astefan added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 17, 2025

elasticsearchmachine merged commit 968b623 into elastic:main Oct 17, 2025
35 checks passed

elasticsearchmachine added the backport pending label Oct 17, 2025

astefan mentioned this pull request Oct 21, 2025

ESQL: Fix double release in inline stats when LocalRelation is reused #136848

Merged

elasticsearchmachine pushed a commit that referenced this pull request Oct 21, 2025

ESQL: Fix double release in inline stats when LocalRelation is reused (…

11e7b4c

…#136848) Backports #136467

astefan removed the backport pending label Oct 22, 2025

		if (stubReplacedSubPlanHolder.get() != null) {
		var plan = stubReplacedSubPlanHolder.get();

ESQL: Fix double release in inline stats when LocalRelation is reused #136467

ESQL: Fix double release in inline stats when LocalRelation is reused #136467

Uh oh!

Conversation

kanoshiou commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 13, 2025

Uh oh!

bpintea commented Oct 14, 2025

Uh oh!

astefan commented Oct 15, 2025

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

kanoshiou commented Oct 15, 2025

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

kanoshiou commented Oct 16, 2025

Uh oh!

astefan commented Oct 16, 2025

Uh oh!

astefan commented Oct 16, 2025

Uh oh!

astefan commented Oct 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astefan left a comment

Choose a reason for hiding this comment

Uh oh!

astefan commented Oct 17, 2025

Uh oh!

kanoshiou commented Oct 17, 2025

Uh oh!

bpintea left a comment

Choose a reason for hiding this comment

Uh oh!

bpintea Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

bpintea Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

bpintea Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

astefan commented Oct 17, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 17, 2025

💔 Backport failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ESQL: Fix double release in inline stats when `LocalRelation` is reused #136467

ESQL: Fix double release in inline stats when `LocalRelation` is reused #136467

kanoshiou commented Oct 13, 2025 •

edited

Loading