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

Added config option max_per_page #1577

Merged
merged 6 commits into from Apr 24, 2024
Merged

Added config option max_per_page #1577

merged 6 commits into from Apr 24, 2024

Conversation

lukaslau
Copy link

Change Summary

Added config option max-per-page and default-topster-size to be able to increase max hits count per page.

Configs max-per-page and default-topster-size value should be the same.

PR Checklist

#780

@kishorenc
Copy link
Member

kishorenc commented Feb 26, 2024

Thank you for the PR. We are fine with making these configurable as a server flag. However, we will have to run some benchmarks to understand whether making these parameters a variable ends up disturbing any compiler optimizations that are currently relying on them being static.

We're in the last leg of the 0.26 release, so will test and benchmark this change in a few weeks.

* Added max_per_page in cofig

* added default topster size config

* removing const default_topster_size

* typo fix

* Update tests.yml

* few test fixes

* bug fix

* test fix

* added couple missing lines

* last test fix
* Added max_per_page in cofig

* added default topster size config

* removing const default_topster_size

* typo fix

* Update tests.yml

* few test fixes

* bug fix

* test fix

* added couple missing lines

* last test fix

* few fixes after updates from origin
@kishorenc
Copy link
Member

@lukaslau

Happy to review this now that the v26 release is done. Can you please rebase this PR against the v27 branch? Thank you.

@lukaslau lukaslau changed the base branch from v0.26-facets to v27 April 20, 2024 10:01
@lukaslau
Copy link
Author

@kishorenc Done

src/collection.cpp Outdated Show resolved Hide resolved
@lukaslau lukaslau changed the title Added config option max_per_page and default_topster_size Added config option max_per_page Apr 23, 2024
Copy link
Member

@kishorenc kishorenc left a comment

Choose a reason for hiding this comment

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

Looks great, just a couple of minor changes and I can merge after that. The most important one is using uint32_t instead of int everywhere for max per page variable.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
include/tsconfig.h Outdated Show resolved Hide resolved
Copy link
Member

@kishorenc kishorenc left a comment

Choose a reason for hiding this comment

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

Thank you!

@kishorenc kishorenc merged commit 2dbbb43 into typesense:v27 Apr 24, 2024
1 check passed
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.

None yet

2 participants