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

feat: Aggregations API #288

Merged
merged 11 commits into from
Jun 9, 2024

Conversation

ditsuke
Copy link
Contributor

@ditsuke ditsuke commented May 19, 2024

What

Adds support for aggregations with a Searcher.aggregate method.

Why

At the moment tantivy-py does not support aggregations in any capacity. This PR adds support for all aggregations with an aggregate method on Searcher which should closely match the existing search method. I also started a discussion (ref) on the Quickwit Community Discord earlier this month to get feedback on the API. While there was not much feedback, the general consensus I think is that we should also eventually expose Collector in tantivy-py to allow for more advanced usage and consistency with the Rust API. While I agree with this strongly (part of the reason I started the discussion), I think it is a good idea to start with this simpler API to get aggregation support going like we have with search.

Note that at the moment the aggregations are returned as a dictionary. I'm still considering whether its worth the effort to port all the types over to pyo3 or if there's another better and simpler way. On the other hand, a dict alone might also work just fine. Please feel free to provide feedback on this.

@ditsuke ditsuke marked this pull request as ready for review May 29, 2024 13:40
@ditsuke
Copy link
Contributor Author

ditsuke commented May 29, 2024

Open for review

@cjrh
Copy link
Collaborator

cjrh commented May 30, 2024

@ditsuke There are many formatting changes in this PR. Could you separate the formatting changes into a different PR? Also, how did you do the formatting, is it just with black, or ruff format or something else?

@cjrh
Copy link
Collaborator

cjrh commented May 30, 2024

Also, while not essential, if you could add a small example in the tutorial section of the docs that would be greatly appreciated. It's up to you if you want, or if you prefer to do that in a separate PR, or not at all :)

https://tantivy-py.readthedocs.io/en/latest/tutorials/

@ditsuke
Copy link
Contributor Author

ditsuke commented May 31, 2024

@cjrh thanks for taking a look. The formatting changes were made by ruff, and while we're on that topic may I suggest deciding on a formatter for the project for consistency? As for now I've reverted those changes (definitely did not belong in this PR). I would like to add the tutorial in a follow up as well.

@cjrh cjrh self-requested a review June 1, 2024 15:09
src/searcher.rs Outdated Show resolved Hide resolved
src/searcher.rs Outdated Show resolved Hide resolved
@cjrh cjrh merged commit 8ece241 into quickwit-oss:master Jun 9, 2024
11 checks passed
@ditsuke
Copy link
Contributor Author

ditsuke commented Jun 9, 2024

Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants