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

Support simple filter syntax in 'all()' method #36

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@bk

bk commented Apr 30, 2015

With this change, the all() method supports the same simplified filtering syntax as the methods it stands in for. The more complex ES syntax is no longer necessary (albeit still supported). For the sake of backwards compatibility, however, this support is not activated unless three parameters are supplied to the method rather than just one or two.

I also added a test stub for the all() method in t/api/all.t.

@mickeyn

This comment has been minimized.

Show comment
Hide comment
@mickeyn

mickeyn Apr 30, 2015

Contributor

@bk thanks.
I'll soon review it.

Contributor

mickeyn commented Apr 30, 2015

@bk thanks.
I'll soon review it.

@mickeyn

This comment has been minimized.

Show comment
Hide comment
@mickeyn

mickeyn May 9, 2015

Contributor

@bk - thanks.
This will produce the correct result set, but not in the way intended (it will fallback to a query rather than filtering on an 'all' query).

You are losing the performance advantage of filtered queries the method 'all' is meant for through using 'query' as 'match_all => {}' and adding a 'filter' part.
Instead, your solution is changing the 'query' part (without building a 'filter') with the restrictions in it (so ES won't cache your query's results)

In case you wish to correct this, I will be glad to assist if you have any questions.

cheers,
Mickey

Contributor

mickeyn commented May 9, 2015

@bk - thanks.
This will produce the correct result set, but not in the way intended (it will fallback to a query rather than filtering on an 'all' query).

You are losing the performance advantage of filtered queries the method 'all' is meant for through using 'query' as 'match_all => {}' and adding a 'filter' part.
Instead, your solution is changing the 'query' part (without building a 'filter') with the restrictions in it (so ES won't cache your query's results)

In case you wish to correct this, I will be glad to assist if you have any questions.

cheers,
Mickey

@bk

This comment has been minimized.

Show comment
Hide comment
@bk

bk May 13, 2015

@mickeyn - sorry for the late reply. Yes, I would like to improve this according to your wishes.

If I understand you correctly, the aim is to support the same kind of simple syntax as for the more specific methods (i.e. module(), distribution() etc.), but to transform it into an efficient ES filter before it reaches the backend.

Since all() delegates how to handle the query to the more specific methods (which ultimately call ssearch() in most cases), this must mean that these methods (or probably simply the _build_body() method in the Request submodule) could be made more efficient as well. Would you like me to look into that?

By the way, do you have any benchmark data for typical queries so I have some point of reference for what should be considered fast, slow - and "fast enough"?

bk commented May 13, 2015

@mickeyn - sorry for the late reply. Yes, I would like to improve this according to your wishes.

If I understand you correctly, the aim is to support the same kind of simple syntax as for the more specific methods (i.e. module(), distribution() etc.), but to transform it into an efficient ES filter before it reaches the backend.

Since all() delegates how to handle the query to the more specific methods (which ultimately call ssearch() in most cases), this must mean that these methods (or probably simply the _build_body() method in the Request submodule) could be made more efficient as well. Would you like me to look into that?

By the way, do you have any benchmark data for typical queries so I have some point of reference for what should be considered fast, slow - and "fast enough"?

@mickeyn

This comment has been minimized.

Show comment
Hide comment
@mickeyn

mickeyn May 14, 2015

Contributor

@bk yes this is the idea (building a filter, using the body builder logic)

Contributor

mickeyn commented May 14, 2015

@bk yes this is the idea (building a filter, using the body builder logic)

@mickeyn

This comment has been minimized.

Show comment
Hide comment
@mickeyn

mickeyn Aug 28, 2016

Contributor

closing as this is doesn't seem to be happening.

Contributor

mickeyn commented Aug 28, 2016

closing as this is doesn't seem to be happening.

@mickeyn mickeyn closed this Aug 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment