Skip to content

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Jul 8, 2025

Fixes a bug during field loading where we could double-close blocks if we failed to allocate memory during the un-shuffling portion of field loading from single segments.

Unit test incoming in the followup.

Closes #130426
Closes #130790
Closes #130791
Closes #130792
Closes #130793
Closes #130270
Closes #130788
Closes #130122
Closes #130827
Closes #130831

Fixes a bug during field loading where we could double-close blocks if
we failed to allocate memory during the un-shuffling portion of field
loading from single segments.

Unit test incoming in the followup.

Closes elastic#130426
Closes elastic#130790
Closes elastic#130791
Closes elastic#130792
Closes elastic#130793
Closes elastic#130270
Closes elastic#130788
Closes elastic#130122
Closes elastic#130827
@nik9000 nik9000 added >non-issue auto-backport Automatically create backport pull requests when merged :Analytics/ES|QL AKA ESQL v9.2.0 v9.1.1 v8.19.1 labels Jul 8, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 8, 2025
@elasticsearchmachine
Copy link
Collaborator

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

assumeFalse("strange exception in the test, fix soon", true);
return ByteSizeValue.ofKb(1);
}

Copy link
Member Author

Choose a reason for hiding this comment

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

This test didn't catch any failures, but it would have if we'd made a non-"simple" version. I'll expand this in a followup I think.

List<Page> input = CannedSourceOperator.collectPages(simpleInput(inputFactoryContext.blockFactory(), between(1_000, 10_000)));

ByteSizeValue memoryLimitForSimple = enoughMemoryForSimple();
Operator.OperatorFactory simple = simple(new SimpleOptions(true));
Copy link
Member Author

Choose a reason for hiding this comment

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

This shift fixed the test.

public int count() {
return docs.getPositionCount();
}
Block[] unshuffled = new Block[target.length];
Copy link
Member Author

Choose a reason for hiding this comment

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

Here's the bug - you can't use the target array and then shuffle into it.

Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing this!

@nik9000
Copy link
Member Author

nik9000 commented Jul 8, 2025

Merging despite the serverless error. It's unrelated and this blocks a bunch of spurious failures.

@nik9000 nik9000 merged commit b34a8c8 into elastic:main Jul 8, 2025
31 of 33 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.1 Commit could not be cherrypicked due to conflicts

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

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Jul 8, 2025
Fixes a bug during field loading where we could double-close blocks if
we failed to allocate memory during the un-shuffling portion of field
loading from single segments.

Unit test incoming in the followup.

Closes elastic#130426
Closes elastic#130790
Closes elastic#130791
Closes elastic#130792
Closes elastic#130793
Closes elastic#130270
Closes elastic#130788
Closes elastic#130122
Closes elastic#130827
@nik9000
Copy link
Member Author

nik9000 commented Jul 8, 2025

9.1 backport: #130848

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Jul 8, 2025
Fixes a bug during field loading where we could double-close blocks if
we failed to allocate memory during the un-shuffling portion of field
loading from single segments.

Unit test incoming in the followup.

Closes elastic#130426
Closes elastic#130790
Closes elastic#130791
Closes elastic#130792
Closes elastic#130793
Closes elastic#130270
Closes elastic#130788
Closes elastic#130122
Closes elastic#130827
nik9000 added a commit that referenced this pull request Jul 8, 2025
Fixes a bug during field loading where we could double-close blocks if
we failed to allocate memory during the un-shuffling portion of field
loading from single segments.

Unit test incoming in the followup.

Closes #130426
Closes #130790
Closes #130791
Closes #130792
Closes #130793
Closes #130270
Closes #130788
Closes #130122
Closes #130827
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/ES|QL AKA ESQL auto-backport Automatically create backport pull requests when merged >non-issue Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) v9.1.1 v9.2.0

Projects

None yet

3 participants