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

Don't use bitset cache for children filters. #10663

Conversation

martijnvg
Copy link
Member

Only parent filters should use bitset filter cache, to avoid memory being wasted.

Closes #10662
Closes #10629

if (filter == null) {
continue;
}
DocIdSet nestedTypeSet = filter.getDocIdSet(context, context.reader().getLiveDocs());
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to pass livedocs?

Copy link
Member Author

Choose a reason for hiding this comment

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

it just felt safe to do this... but I don't we need to, because nested docs can't be modified on their own. Their life cycle depends on the parent doc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its a little confusing, since we explicitly didn't pass bits before. IMO we should just have a code comment

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 added a comment regarding this.

}
// We can pass down 'null' as acceptedDocs, because nestedDocId is a doc to be fetched and
// therefor is guaranteed to be a live doc.
DocIdSet nestedTypeSet = filter.getDocIdSet(context, null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Filter.getDocIdSet can return null, can you check that nestedTypeSet is not null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also the iterator.

@jpountz
Copy link
Contributor

jpountz commented Apr 24, 2015

LGTM

@jpountz
Copy link
Contributor

jpountz commented Apr 24, 2015

I just left a comment about some missing null checks.

@martijnvg martijnvg force-pushed the inner_hits/use_bitset_cache_only_for_parent_filters branch from 3687d9b to 0ad1c12 Compare April 30, 2015 14:07
Only parent filters should use bitset filter cache, to avoid memory being wasted.
Also in case of object fields inline the field name into the nested object,
instead of creating an additional (dummy) nested identity.

Closes elastic#10662
Closes elastic#10629
@martijnvg martijnvg force-pushed the inner_hits/use_bitset_cache_only_for_parent_filters branch from 79ca16d to 7a6fe80 Compare April 30, 2015 15:01
@martijnvg martijnvg merged commit 7a6fe80 into elastic:master Apr 30, 2015
@martijnvg martijnvg deleted the inner_hits/use_bitset_cache_only_for_parent_filters branch May 18, 2015 23:25
@clintongormley clintongormley changed the title inner_hits: Don't use bitset cache for children filters. Don't use bitset cache for children filters. May 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Elasticsearch inner_hits query does not return second nested field in object field
5 participants