Skip to content

Use cutoff_frequency everywhere#88

Merged
orangejulius merged 3 commits intomasterfrom
cutoff_frequency
Jan 14, 2019
Merged

Use cutoff_frequency everywhere#88
orangejulius merged 3 commits intomasterfrom
cutoff_frequency

Conversation

@orangejulius
Copy link
Copy Markdown
Member

@orangejulius orangejulius commented Oct 17, 2018 •
edited
Loading

Allow query views to set the cutoff_frequency feature to support better performance on queries with lots of high-frequency terms.

See pelias/api#1213 for more explanation on the use cases.

@orangejulius orangejulius mentioned this pull request Oct 17, 2018
Merged
@missinglink
Copy link
Copy Markdown
Member

missinglink commented Jan 14, 2019

@orangejulius I have actioned the todo's you noted above, the extra commit modifies the behaviour of cutoff_frequency so that it can be defined on a per-field basis for sub-properties of address and admin.

@missinglink
Copy link
Copy Markdown
Member

missinglink commented Jan 14, 2019

This is good to merge, we just need to be careful that default values are set for each possible property combination before merging or the views will start failing.

missinglink
missinglink approved these changes Jan 14, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@missinglink missinglink left a comment

Choose a reason for hiding this comment

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

Good-to-merge pending default values being set for these properties in pelias/api.

@orangejulius
Copy link
Copy Markdown
Member Author

orangejulius commented Jan 14, 2019

Nice, thanks. Another option is to make this PR a noop if those values aren't defined. Then we could merge it immediately and handle the rest in API.

@missinglink
Copy link
Copy Markdown
Member

missinglink commented Jan 14, 2019

Good call, I've added another commit which now makes cutoff_frquency optional, when not defined it simply doesn't add that property to the query.

@missinglink
Copy link
Copy Markdown
Member

missinglink commented Jan 14, 2019

@orangejulius feel free to merge this if you're happy with it.

@orangejulius orangejulius changed the title WIP: use cutoff_frequency everywhere Use cutoff_frequency everywhere Jan 14, 2019
@orangejulius
Copy link
Copy Markdown
Member Author

orangejulius commented Jan 14, 2019

I've added to the last commit to make cutoff_frequency optional everywhere, and in my testing this PR is indeed a noop on pelias/api.

@orangejulius orangejulius merged commit e141f2b into master Jan 14, 2019
@orangejulius orangejulius deleted the cutoff_frequency branch January 14, 2019 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@missinglink missinglink missinglink approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@orangejulius @missinglink