API proposal : advanced numeric filters

bobylito commented 9 years ago

TL;DR : Algolia can do much more than what is actually possible with the helper! We need to refresh the API.

Updates of this issue

as suggested by @redox, the current behaviour of the add methods should be treated as a bug
if the user passes an array it will make OR predicates, otherwise the use of multiple add make them AND predicates
State of the API's

On the Algolia side

The Algolia engine supports pretty advanced queries with two different type of predicates and let the user combine them with ANDs and ORs.

We have to two types of predicates that is supported :

operator based predicates : a given attribute can be filtered with the use of an operator and a value. The supported operators are=, !=, >, >=, < and <=. For example, given that we have two numeric attributes a and b, we can do :

a!=3 AND (b=2 OR b>27)

range based predicates. No operators, just the range and the attribute. The single value is a special range with only value. They can be combined the same way as above. For example :

a:3 AND b:2 to 27 #this is not the same example

The last type of predicates can easily be emulated with operators.

The helper side

The Helper provides an API to set operator based predicates with some limitations :

You can only combine predicates with ANDs
For a given pair of attribute-operator you can only set one value

Examples :

# possible 
a=31 AND a!=22 AND b >2

# not possible /!\
a=3 AND a=2
a=3 OR b=42

Enhancing the API

This proposition comes with certain aspects in mind :

be backward compatible as much as possible
fix inconsistencies
extend as much as we can without making the API too weird (lots of interpretation and personal gut feeling around this one :P)
old stuff should be as easy to do as before
Extension of the current API

We have a set of methods (SearchParameters) to do operations on the numeric filters : add, remove, isRefined, get. They are either exposed or combined on the Helper :

// helper is already init
helper.addNumericRefinement('a', '=', 3); // Add the refinement
helper.addNumericRefinement('a', '=', 5); // Update the refinement=5
helper.getRefinement('a');  // returns {"=" : "3"}
helper.removeNumericRefinement('a', '='); // Remove the refinement for (a, =)

The current behavior is sort of a bug, as we use it to update. The semantic must change to ensure consistency with the other API and with the name itself. That's why the add will just add and not update anymore.

helper.addNumericRefinement('a', '=', 1); // Add the refinement a=1
helper.addNumericRefinement('a', '=', 2); // Add the refinement a=2
// at that point the resulting numericFilters=a:1,a:2  <=>  (a:1 AND a:2)
helper.getRefinement('a');  // returns {"=": [1, 2]}

helper.addNumericRefinement('a', '=', [3, 4]); // Add the refinement a=3 OR a=4
// at that point the resulting numericFilters=a:1,a:2,(a:3,a:4)  <=>  (a:1 AND a:2 AND (a:3 OR a:4))

helper.getRefinement('a');  // returns {"=": [1, 2, [3, 4]]}

Introducing a raw API

The same way we can set the tags as algolia understand them in a string form : letting the user set the algolia parameter numericFilters. Previously this parameter was computed. The main characteristics are :

can't use both the other API and this one, to avoid collisions
no help is provided as for the handling or serializing those filters
supports everything that Algolia could understand now or in the future

Example :

helper.setQueryParameter("numericFilters", "a:3 to 5,(b : 2, b:5)");
helper.setQueryParameter("numericFilters", "a>=3, b<5,(b = 2, b = 5)");

TODO

[x] Modify the current API
[x] Implement the advanced API
[x] Document the changes of the API in the README
[x] Changelog

bobylito commented 9 years ago

@redox @vvo @maxiloc @kokliKo @Jerskouille @purban @seafoox feel free to comment, propose or cheer :) Will update the issue accordingly :)

redox commented 9 years ago

I think it's too different from what we have today, the "update" feature is definitely not intuitive compared to the other refinements:

helper.addFacetRefinement('a', 'bar'); // Add the refinement a:bar
helper.addFacetRefinement('a', 'foo'); // Add the refinement a:foo (no update here)

// VS

helper.addNumericRefinement('a', '=', 1); // Add the refinement
helper.addNumericRefinement('a', '=', 2); // Update the refinement

I would rather propose:

helper.addNumericRefinement('a', '=', 1); // Add the refinement a=1
helper.addNumericRefinement('a', '=', 2); // Add the refinement a=2
// at that point the resulting numericFilters=a:1,a:2  <=>  (a:1 AND a:2)
helper.getRefinement('a');  // returns {"=": [1, 2]}

helper.addNumericRefinement('a', '=', [3, 4]); // Add the refinement a=3 OR a=4
// at that point the resulting numericFilters=a:1,a:2,(a:3,a:4)  <=>  (a:1 AND a:2 AND (a:3 OR a:4))

helper.getRefinement('a');  // returns {"=": [1, 2, [3, 4]]}

I'm happy with the raw setQueryParameter.

bobylito commented 9 years ago

@redox I agree it differs from the facet API's but it is similar to the current (almost identical) to the current numeric API. Making it the way you are proposing would makes it backward incompatible. Here is an example :

helper.addNumericRefinement('a', '=', '3'); // current : a = 3, yours : a = 3, mine : a = 3
helper.addNumericRefinement('a', '=', '5'); // current : a = 5, yours : a = 5 and a = 3, mine : a = 5

redox commented 9 years ago

Making it the way you are proposing would makes it backward incompatible.

For me it's a "bug" that the numeric refinement is currently updated, so I would not care about the backward compatibility here.

bobylito commented 9 years ago

OK. Seeing things that way solves everything :) I have a gutt feeling that this will be hard for me to use in a implementation but I want to test. I will update the issue with your proposal.

bobylito commented 9 years ago

@redox one thing though, how do I get to update the or values? Do you think I should clear everything and then add them back?

vvo commented 9 years ago

For me it's a "bug" that the numeric refinement is currently updated, so I would not care about the backward compatibility here.

Since this is the behavior for some months, it might be a feature for a good number of users I would still introduce this in a breaking change.

For example I found this codepen with a google search that shows a user doing this:

// in an init function:
helper.addNumericRefinement('best_offer.price_with_vat', '<=', value);

// later on in an event handler:
helper.addNumericRefinement('best_offer.price_with_vat', '<=', ev.value)

Which seems to indicate he's relying on the update "bug". And this was only a simple google search, I guess it's already present in other codebases.

bobylito commented 9 years ago

This will be marked as breaking in the changelog for sure.

vvo commented 9 years ago

If it breaks it goes to 3.0.0, because we advocate for the use of /2/algoliasearch.js which will auto update and potentially break clients. right?

bobylito commented 9 years ago

You are right... If it's not a bug. That's the only loophole that could get us not do new major version.

bobylito commented 9 years ago

@vvo's argument is just too strong, we need to stick to semver completely if we want to the version to mean something... Let's go for 3.0.0!

redox commented 9 years ago

Except if we see that change as a bug fix, and from all the feedback I had about it: this behavior was more experienced as a bug than a feature.

redox commented 9 years ago

Except if we see that change as a bug fix, and from all the feedback I had about it: this behavior was more experienced as a bug than a feature.

I think that change is "soooo minor". I understand it's not good to change a behavior without bumping the major version but here I'm convinced it was more a bug than a feature. Having a major upgrade for "just" that will generate a LOT of migration questions, and users will be afraid of migrating.

I don't have a stronger argument but I think it would exactly be the same question if it was a bug: "if fixing a bug changes the behavior of something, is that still a patch bump?

bobylito commented 9 years ago

If we look at the semver.org website :

Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards compatible bug fixes are introduced. A bug fix is defined as an internal change that fixes incorrect behavior.

Minor version Y (x.Y.z | x > 0) MUST be incremented if new, backwards compatible functionality is introduced to the public API. It MUST be incremented if any public API functionality is marked as deprecated. It MAY be incremented if substantial new functionality or improvements are introduced within the private code. It MAY include patch level changes. Patch version MUST be reset to 0 when minor version is incremented.

Major version X (X.y.z | X > 0) MUST be incremented if any backwards incompatible changes are introduced to the public API. It MAY include minor and patch level changes. Patch and minor version MUST be reset to 0 when major version is incremented.

The answer is yes we should bump a major version. Let's settle that when we actually make the release :)

gomesNazareth commented 7 years ago

Can I make a search like below (a <=1000 AND a<=5000) OR (a<=7000 AND a<9000) This is for a side filter where I can select number of different salary ranges.

I have tried doing it in the below manner but its not working.Please can anyone help this.helper.setQueryParameter("numericFilters",[jobseeker_profile.general_info.total_years_experience:1000 to 5000 , jobseeker_profile.general_info.total_years_experience:7000 to 9000]");

Jerska commented 7 years ago

The array syntax here will be considered as an AND. If I recall correctly though, nested arrays are considered as ORs, so I think you should be able to achieve what you want with:

this.helper.setQueryParameter("numericFilters", [
  [
    'jobseeker_profile.general_info.total_years_experience:1000 to 5000',
    'jobseeker_profile.general_info.total_years_experience:7000 to 9000'
  ]
])

Edit:

After some discussions with @kokliKo , I was doubting. Indeed, the engine doesn't allow you to use OR of ANDs, and we were unsure whether ranges were considered as ANDs. After creating a test index, I confirm this works. :)

gomesNazareth commented 7 years ago

@Jerskouille Thank you for the help .The syntax you have given is working fine. Is there a way to do the same thing using the addNumericRefinement filter. Actually i used addNumericRefinement for all other search filters. But if addNumericRefinement is used along with setQueryParameter. It throws an error that says.

Error: [Numeric filters] Can't switch from the managed API to the advanced. It is probably an error, if this is really what you want, you have to first clear the numeric filters.

Jerska commented 7 years ago

Unfortunately, it's either one or the other. You'll likely need to build you own numericRefinements here. The only thing to know is that items inside the first array will be ANDed, and items in a nested array will be ORed.

gomesNazareth commented 7 years ago

@Jerskouille Thanks for replying . I removed addNumericRefinement from every where and used only setQueryParameter. Is it possible to filter on date 2017-05-29T06:06:51.362Z ?

Jerska commented 7 years ago

Just be careful, setQueryParameter will override previous numericRefinements, not add them.

To handle time, you should use numeric timestamps instead, and filter with those numbers.

gomesNazareth commented 7 years ago

@Jerskouille thanks for the help

gomesNazareth commented 7 years ago

@Jerskouille I am trying to get all the records . I am currently showing 10 records per page . But I need to get the total records for the search to show the page numbers and also to show the total records for that particular search. Currently to solve this issue I run algolia 2 times .One with pagination and other without pagination and (attributesToRetrieve: null). Is there a better way to do this ?

gomesNazareth commented 7 years ago

ok I realised I can use nbHits to get the total records . but i noticed some times nbHits and hits don't match .

Jerska commented 7 years ago

This is unrelated to this issue, please open a new one next time, or post directly on https://discourse.algolia.com/ . :)

hits contains maximum the 10 ones you chose to display with hitsPerPage. nbHits contains the total amount of hits matching the search. There's also nbPages in the response.

You can check out the code for the pagination in this instantsearch.js file: https://github.com/algolia/instantsearch.js/blob/2bd93adaf9dde42e52407d9a75190045bb45d778/src/widgets/pagination/pagination.js .

gomesNazareth commented 7 years ago

@Jerskouille I am sorry for posting an unrelated issue here. Next time I will create a new issue for it

algolia / algoliasearch-helper-js