[arnesetzer]

👋 I did some awesome work for the Pelias project and would love for everyone to have a look at it and provide feedback.

This is the first of two PRs who needs to be merged together. The second one is here: pelias/api#1725

---

Here's the reason for this change 🚀

For a project I need the functionality to exclude categories like layers which is currently not possible. Maybe it is a useful thing to have on upstream.

---

Here's what actually got changed 👏

• view/categories.js: Add support for excluding categories by adding a "must_not" array
• test/view/cateogries.js: Tests

---

Here's how others can test the changes 👀

• Run the tests

[Joxit]

Hi,
Thank you for using pelias and submitting PR 😄
I put some comments in your code.
Could you also keep the same indentation / code style on all unchanged lines ? This will help us for the review and reduce the number of unnecessary changes.

[arnesetzer]

Fixed the mix up. Sorry again. After running npm run lint without any errors/warnings I hope the code is properly intended.

[Joxit]

Better than the first PR 👍

[Joxit]

You could have keep the view with something like

const view = { bool: { } };

And then view.bool.must = [ terms(...) ] or view.bool.must_not = [ terms(...) ]
Thanks to this you will have only two conditions and one return

[Joxit]

lodash _.concat(value) is doing the same, but IDK which one is better

[Joxit]

You can split this in two lines, first one being const value = asArray(...) if you think this is too long

[orangejulius]

Hey @arnesetzer thanks for the PR.

Two concerns here.

First, I just want to make sure I understand the use case here. I'm guessing you want to be able to form queries like "every type of venue except transit" or "every amenity subcategory except amenity:bbq". Because every record can have multiple categories, and at least in our sample OSM category mapping, categories have a hierarchy structure, there might be a lot more variations with how this can play out than the layers param.

Second, adding new parameters is a tough one because they either have to be provably okay for performance in all cases (like negative layers was), or behind a pelias.jon flag to enable/disable them.

Using must_not is known to have potential performance issues because it can't use the inverted index, so it's an additional filter step across every document returned. For negative layers, we implement it as a must by utilizing the already known list of possible layers to work around this.

There are generally lots more possible category values than layers values, so the same approach could work, but it's a bit unweildy if the elasticsearch queries we are generating might have a must with a couple hundred values. But that would be one way to be able to enable negative categories unconditionally.

I'm sure you're groaning at the idea of adding a config flag to enable/disable this parameter, so I'm now kinda wondering if we should build a system where any parameter that isn't required can be disabled in pelias.json. It would give people running Pelias a lot more configurability and avoid us maintainers having to give the "your niche query parameter is interesting but needs all this extra work" speech again, which we don't love doing either. 😆 And then when those flags do get implemented, it's at least consistent.

[arnesetzer]

First, I just want to make sure I understand the use case here. I'm guessing you want to be able to form queries like "every type of venue except transit" or "every amenity subcategory except amenity:bbq". Because every record can have multiple categories, and at least in our sample OSM category mapping, categories have a hierarchy structure, there might be a lot more variations with how this can play out than the layers param.

More or less, we import the categories as venues via the CSV importer and use the categories (and addendum_json) to filter them (different icons in the search results, etc.). For a specific search, we want to exclude certain categories. I thought it would be the same with layers ('-xyz' excludes 'xyz'), but this was not the case. As we needed this kind of filtering anyway and did not want to filter on the client side, I implemented this solution.

Second, adding new parameters is a tough one because they either have to be provably okay for performance in all cases (like negative layers was), or behind a pelias.jon flag to enable/disable them.

Using must_not is known to have potential performance issues because it can't use the inverted index, so it's an additional filter step across every document returned. For negative layers, we implement it as a must by utilizing the already known list of possible layers to work around this.

There are generally lots more possible category values than layers values, so the same approach could work, but it's a bit unweildy if the elasticsearch queries we are generating might have a must with a couple hundred values. But that would be one way to be able to enable negative categories unconditionally.

I was not aware of this. When using only custom POIs (approximately 10⁶) on a relatively small index (<10⁹ entries in Elasticsearch), I did not recognise any performance issues. However, you are right that using the OSM categories for the whole world will significantly increase the size and probably slow down the geocoder.

I'm sure you're groaning at the idea of adding a config flag to enable/disable this parameter, so I'm now kinda wondering if we should build a system where any parameter that isn't required can be disabled in pelias.json. It would give people running Pelias a lot more configurability and avoid us maintainers having to give the "your niche query parameter is interesting but needs all this extra work" speech again, which we don't love doing either. 😆 And then when those flags do get implemented, it's at least consistent.

Just to clarify, are we talking about parameters such as 'layer', 'category' and 'alt_name' (which, if I recall correctly, is not currently indexed at all), or specific categories? If it's the latter, my case would still be valid, since I need to enable and disable specific categories dynamically.