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

Source filtering in new field collapsing feature (Elasticsearch 5.3) #24063

Closed
byronvoorbach opened this issue Apr 12, 2017 · 8 comments

Comments

@byronvoorbach
Copy link
Contributor

commented Apr 12, 2017

Hi.

While trying out the new 'field collapsing' feature in ES 5.3, I noticed that source filtering is handled differently than when source filtering is applied inside the top query element. I created a gist to show off an example:

https://gist.github.com/byronvoorbach/bc3d73c77aaa7ddaa96ee47a9ba886fc

In short: 'collapse' feature needs an explicit include/exclude during source filtering where as this not needed when using source filtering on top level documents

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2017

@byronvoorbach good catch, do you wanna try to work on a PR for this, we are happy to help if you are up for it?

@byronvoorbach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

Hey @s1monw!

I'm leaving for Sweden the next couple of days, so won't have time for it till after the Easter weekend. Would be happy to work on a PR when I come back if that's not too late for you guys!

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2017

@byronvoorbach just check in when you come back and see if it's been worked on... we will see how quickly we can get to it. You are welcome to help. Enjoy the trip ;)

@byronvoorbach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

@simonw Will do, wouldn't mind helping out! And thanks!

@byronvoorbach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

@simonw you got me excited to look into the codebase, and I found the issue. Just a single line change is needed to get it to work. (except for failing tests after)

I'll see if I can fix the tests and create a PR tonight (before I leave) and if not post my code finding here

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2017

no go ahead and open a PR please @byronvoorbach

byronvoorbach added a commit to byronvoorbach/elasticsearch that referenced this issue Apr 12, 2017
elastic#24063 - Update ValueType of FetchSourceContext to OBJECT_ARRA…
…Y_OR_STRING to be in line with other source_filtering implementations
byronvoorbach added a commit to byronvoorbach/elasticsearch that referenced this issue Apr 12, 2017
@byronvoorbach

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

@s1monw Done! #24068

byronvoorbach added a commit to byronvoorbach/elasticsearch that referenced this issue May 9, 2017
elastic#24063 - Added 'OBJECT_ARRAY_BOOLEAN_OR_STRING' to ObjectParse…
…r for improved source filtering in InnerHitsBuilder. Fixed tests accordingly
@clintongormley

This comment has been minimized.

Copy link
Member

commented May 15, 2017

Closed by #24068

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.