Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESQL: Track memory from values loaded from lucene #101383

Merged
merged 8 commits into from
Oct 27, 2023

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Oct 26, 2023

This adds memory tracking for values loaded from doc values and stored fields.

This adds memory tracking for values loaded from doc values and stored
fields.
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-ql (Team:QL)

@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/elasticsearch-esql (:Query Languages/ES|QL)

Copy link
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.

I'm going to push an extra change here - I'll flip the tests from defaulting to an untracked BlockFactory to a tracked one. We want all tests to use a tracked one in soon enough, so let's get the default right.


@Override
public void close() {
// TODO assert that we close the test blocks
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look at this one now.

growValuesArray(newSize);
adjustBreaker(-valuesLength * elementSize());
Copy link
Member Author

Choose a reason for hiding this comment

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

I made this change while trying to reenable one of the HeapAttack tests - specifically the too many mv fields one. It didn't get it passing, which is genuinely odd to me. Either way, I think it is more correct.

Copy link
Member

Choose a reason for hiding this comment

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

++ this is more correct as we create a new array of newSize before copying.

@@ -161,6 +161,7 @@ public static Block[] fromList(BlockFactory blockFactory, List<List<Object>> lis
public static Block deepCopyOf(Block block, BlockFactory blockFactory) {
try (Block.Builder builder = block.elementType().newBlockBuilder(block.getPositionCount(), blockFactory)) {
builder.copyFrom(block, 0, block.getPositionCount());
builder.mvOrdering(block.mvOrdering());
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 was required to get some tests working. It meant I had to make the method a noop on unsupported block types but that seems ok.

}
}
}
// for (Page p : results) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Woop.s Let me get this back.

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.

growValuesArray(newSize);
adjustBreaker(-valuesLength * elementSize());
Copy link
Member

Choose a reason for hiding this comment

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

++ this is more correct as we create a new array of newSize before copying.

It just started failing. Let's suppress it in this PR.
@nik9000 nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Oct 26, 2023
@elasticsearchmachine elasticsearchmachine merged commit 5365daa into elastic:main Oct 27, 2023
@nik9000 nik9000 deleted the esql_memory_on_values branch October 27, 2023 01:25
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-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >enhancement Team:QL (Deprecated) Meta label for query languages team v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants