Better ranked search results in app list #561

pierluigi · 2016-01-19T16:11:16Z

ACs

First result should be the exact app name match.
Followed by deepest group match first.

From:

To:

aldipower · 2016-01-19T17:22:02Z

src/js/components/AppListComponent.jsx

@@ -186,7 +198,11 @@ var AppListComponent = React.createClass({

    if (filterText != null && filterText !== "") {
      nodesSequence = nodesSequence
-        .filter(app => app.id.indexOf(filterText) !== -1);
+        .filter(app => {
+          app.filterScore = score(app.id, filterText);


You are modifying the app here, this should be done only in the store (with emiting an event afterwards).
Also this is done in a filter. It's not expectable that a filter modifies data.
So this isn't the right approach. Sorry. :)

Let's try to sort and score the list where the sorting belongs to.
Could you add a sort function after the .sort()-function at line 316?

if (filterText != null) { nodesSequences.sort((a, b) => { return score(a.id, filterText) > score(b.id, filterText) ? -1 : 1; }); }

Didn't tested it, but it should go into this direction.
Maybe there is also a more efficient way by integrating this into the already existing sort-function. Not sure.

This doesn't modify the app itself. :)

You're totally right. Will follow your suggestions.

That won't work, however, if we plan on supporting manual overriding of the sorting. I would like, for example, to search for phil and get the ranked sorting. But if I then click on one of the sorting columns, (for example CPU), this should override the search rank.
My solution is hacky so it's a no-go, so let's have a chat about a better solution that addresses all of that.

add a simple condition to the if should do the trick:

if (filterText != null && sortKey !== "id") {

That means i can't sort my search results alphabetically?

Alphabetically sorting is done before.
https://github.com/mesosphere/marathon-ui/blob/master/src/js/components/AppListComponent.jsx#L315

The sort-function mentioned above should be added below that.

I did a mistake in the condition, must be:

if (filterText != null && sortKey === "id") {

Sorry, the link goes to the wrong view type I think. May bad.
But doesn't change the principle how it works.

An additional sort function should be added here:

https://github.com/mesosphere/marathon-ui/blob/master/src/js/components/AppListComponent.jsx#L301

aldipower · 2016-01-19T17:23:18Z

A test would be neat! :)

aldipower · 2016-01-19T17:29:16Z

According to the branchname, this is actually a feature and not a fix.
But this is very very minor nitpicking.

aldipower · 2016-01-19T17:37:38Z

I had a look at the fuzzaldrin code.
It looks slightly inefficient and it's not because it is coffescript. :)
A lot of replaces and matches there.
https://github.com/atom/fuzzaldrin/tree/master/src
I wonder if this works together without our scalability objective, when we will have a lot of apps.

Is there is maybe a simpler solution?

Maybe just taking a second sorting round only on the applications name without the groups-portion?
Then we had 2 sort steps (like with fuzzaldrin anyways).

Sorting on the complete id -> this affects mostly the sorting on the groups segements (like it is already)
Sorting only at the application name -> kind of a ranking

Sounds simple. Do I miss something?

pierluigi · 2016-01-19T18:39:33Z

I picked fuzzAldrin specifically because it adds fuzzy matching:

This library is used by Atom and so its focus will be on scoring and filtering paths, methods, and other things common when writing code. It therefore will specialize in handling common patterns in these types of strings such as characters like /, -, and _, and also handling of camel cased text.

That is a very useful feature to have in our toolbelt, imho. Imagine searching very nested appIds, without the need to be precise.

Regarding performance, I will do some tests. Surely indexOf is more performant, but this does not look bad at all:

Filtering 66672 entries for 'index' took 146ms for 6168 results
Matching 66672 entries for 'index' took 140ms for 6168 results

(I ran npm run benchmark in the fuzzaldrin source code repo).

We are only using the score function by the way.

I agree we should add some tests, and I'll do that next.

pierluigi · 2016-01-20T11:27:39Z

@aldipower I've squashed the changes, please take another look now.

Here's a gist that helps you generate a number of fake apps for testing performance and ranking: https://gist.github.com/pierlo-upitup/c325692b3e9511da6590

pierluigi · 2016-01-20T11:29:28Z

Should we disable the sorting caret for the Name column when filterText != null?

aldipower · 2016-01-20T12:30:15Z

Thanxman! Going to take a second round now.
Having problems to install the modules on the first try. I think the shrinkwrap is faulty.

pierluigi · 2016-01-20T12:39:06Z

Oh yeah. I need to re-generate it...

aldipower · 2016-01-20T12:40:21Z

src/js/components/AppListComponent.jsx

@@ -186,7 +187,10 @@ var AppListComponent = React.createClass({

    if (filterText != null && filterText !== "") {
      nodesSequence = nodesSequence
-        .filter(app => app.id.indexOf(filterText) !== -1);
+        .filter(app => {
+          return score(app.id, filterText) > 0.025;


Why is this needed? It leads to unexpected behavior.

Before (like it was), I see all result (ranked with the sorting later):

👍

After - one result is hidden, I don't expect this:

👎

Let's stick with indexOf() and keep it simple.

It's just a matter of tweaking that 0.25 to something lower, maybe 0.2.

But what sense?
And what else is hidden then unexpected?
Cannot detect the use case for the scoring here.

The score value is our way of determining matches in the app list for the given filterText. https://github.com/atom/fuzzaldrin#scorestring-query

If we move back to indexOf we lose the whole fuzz matching algorithm completely! Why would you want to do that? I agree that the result not showing up was a regression, but that's because i set the score threshold too high. Please try again now.

OK, now I got it. You want fuzzy matched results, besides direct matches.
This makes sense.
I was confused because this behavior isn't reflected in the related issue.
It only says that the given results should be ranked better. That's how I understand it.

With this, now, you modify the complete result set.
But I like it!

Confirmative. :)

aldipower · 2016-01-20T12:50:58Z

@pierlo-upitup OK! Review completed. :)
Some things need a little bit rework, but after that I am going to merge this.

I am fine by going with fuzzaldrin, he is doing a good job. :) And my solution I thought about is too simple, doesn't really work out.

This makes the search result way meaningful.
Thanx for the work @pierlo-upitup !

pierluigi · 2016-01-20T12:54:32Z

Thanks for that! I just squashed your requested changes and especially tweaked the score value to 0.02 – that should be fine! Please let me know your thoughts.

FTR: I think at some point it might be a really good idea to move this whole filtering logic to the AppsStore or to a new ApssFilterStore because this component is doing far too much!

aldipower · 2016-01-20T12:56:21Z

Thx, please look at my comment regarding the "0.02".
Don't get the use case for that.
Let's keep it simple please.

aldipower · 2016-01-20T13:07:47Z

CHANGELOG.md

@@ -16,6 +16,7 @@
 - \#2831 - Convert alert, confirm and prompt dialogs to new design
 - \#2909 - Update dialog messages
 - \#2655 - Never preselect dangerous actions in modal dialogs
+- \#3018 - Better search result ranking


Could you be a little bit more precise here? That we do a fuzzy search.

aldipower · 2016-01-20T13:15:29Z

Cool!
So, maybe a little bit more precise changelog, a test and a rebase is left.
hurray

pierluigi · 2016-01-20T14:38:59Z

I have removed the caret from the Name column when a filterText is present, as I believe it only generates confusion (you don't order search results on Google...). I've also added some tests. We should be good now!

pierluigi · 2016-01-20T15:02:49Z

@aldipower please take a look at this, for a clarification as to why I think we should not allow Name sorting when showing ranked search results:

When i click on the "Name" column, as a user I expect the results sorting by Name. But what this accomplishes, is to sort results by rank in the opposite direction instead.

aldipower · 2016-01-20T15:06:44Z

@pierlo-upitup Yeah, got what you mean, I would add a hover text on the caret, that explains the sorting by score.
I like to have this functionality of sorting the name column by score.
"Name" is the title of the column, not the definition of sorting the column by id alphabetically.

aldipower · 2016-01-20T15:07:54Z

The health column is also not alphabetically sorted, but ranked! Same thing. Ok, slightly.. ;)

pierluigi · 2016-01-20T15:10:20Z

The Name column works exactly on the id value itself outside of the search page.
Why should that behaviour change in this context to work on a completely different and invisible value (score)? The tooltip explanation feels like a workaround to me, and changing the behaviour of a sorting column is bad UX.

Sorting by the healthbar is consistent across all other pages, so it's not a good comparison.

Why would a user you want to see the worst ranking results first, anyway?

pierluigi · 2016-01-20T15:15:39Z

Take a look at Google Drive.The same is true: sorting by Name is disabled in the search results page.

aldipower · 2016-01-20T15:15:54Z

OK, the context change is a good argument to me. Thank you for argue this out. The healthbar example wasn't good.
Disabling the caret is also a workaround, but I think probably the better one.

aldipower · 2016-01-20T15:19:49Z

src/js/components/AppListComponent.jsx

+    var filterText = this.props.filters[FilterTypes.TEXT];
+
+    if (sortKey === this.state.sortKey &&
+      !(sortKey === "id" && filterText != null && filterText !== "")) {


Sorry for bothering you, could we negate this?

(sortKey !== "id" || filterText == null || filterText === "")) {

forcepushed, sir!

aldipower · 2016-01-20T15:24:29Z

Thank you for the tests, very cool!

pierluigi · 2016-01-20T15:26:15Z

Aahaha and of course coveralls.io decided to die on us on the first PR with some tests 😛

aldipower · 2016-01-20T15:26:49Z

LOL

aldipower · 2016-01-20T16:03:26Z

Great!!!1!ELEVEN!!
Thanx a lot for your patience on my comments.

Very well done!

This will be merged now.

This fixes mesosphere/marathon#3018

The Name column should not be sortable when looking at ranked search results

aldipower · 2016-01-20T16:20:58Z

Mission accomplished!

Better ranked search results in app list

pierluigi assigned aldipower Jan 19, 2016

pierluigi added the in progress label Jan 19, 2016

aldipower reviewed Jan 19, 2016
View reviewed changes

pierluigi force-pushed the fix/3018-ranked-search branch from ac4eede to bd834f6 Compare January 20, 2016 11:26

aldipower reviewed Jan 20, 2016
View reviewed changes

pierluigi force-pushed the fix/3018-ranked-search branch from bd834f6 to d0a4538 Compare January 20, 2016 12:52

aldipower reviewed Jan 20, 2016
View reviewed changes

pierluigi force-pushed the fix/3018-ranked-search branch from d0a4538 to 5ca3720 Compare January 20, 2016 14:37

aldipower reviewed Jan 20, 2016
View reviewed changes

pierluigi force-pushed the fix/3018-ranked-search branch from 5ca3720 to 9c6f965 Compare January 20, 2016 15:24

pierluigi force-pushed the fix/3018-ranked-search branch from 9c6f965 to 2a0b491 Compare January 20, 2016 15:25

Pierluigi Cau added 7 commits January 20, 2016 17:06

Better ranking of search results

5077c45

Add CHANGELOG entry

113c48f

This fixes mesosphere/marathon#3018

Adjust order of package.json

436cad2

Prevent sorting ranked results

85e384d

The Name column should not be sortable when looking at ranked search results

Add tests for App List component

20c5a34

Tweak score threshold for better results

adcc5fd

Add test for edge case with long appId

4215d80

pierluigi force-pushed the fix/3018-ranked-search branch from 464a932 to 4215d80 Compare January 20, 2016 16:07

aldipower added a commit that referenced this pull request Jan 20, 2016

Merge pull request #561 from mesosphere/fix/3018-ranked-search

5e612c6

Better ranked search results in app list

aldipower merged commit 5e612c6 into master Jan 20, 2016

aldipower deleted the fix/3018-ranked-search branch January 20, 2016 16:21

aldipower removed the in progress label Jan 20, 2016

Better ranked search results in app list #561

Better ranked search results in app list #561

Conversation

pierluigi commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldipower commented Jan 19, 2016

aldipower commented Jan 19, 2016

aldipower commented Jan 19, 2016

pierluigi commented Jan 19, 2016

pierluigi commented Jan 20, 2016

pierluigi commented Jan 20, 2016

aldipower commented Jan 20, 2016

pierluigi commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldipower commented Jan 20, 2016

pierluigi commented Jan 20, 2016

aldipower commented Jan 20, 2016

Choose a reason for hiding this comment

aldipower commented Jan 20, 2016

pierluigi commented Jan 20, 2016

pierluigi commented Jan 20, 2016

aldipower commented Jan 20, 2016

aldipower commented Jan 20, 2016

pierluigi commented Jan 20, 2016

pierluigi commented Jan 20, 2016

aldipower commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldipower commented Jan 20, 2016

pierluigi commented Jan 20, 2016

aldipower commented Jan 20, 2016

aldipower commented Jan 20, 2016

aldipower commented Jan 20, 2016