Skip to content

Improve search queries#755

Merged
jamiehewitt15 merged 8 commits intomainfrom
issue-678-improve-search
Aug 2, 2021
Merged

Improve search queries#755
jamiehewitt15 merged 8 commits intomainfrom
issue-678-improve-search

Conversation

@jamiehewitt15
Copy link
Contributor

@jamiehewitt15 jamiehewitt15 commented Jul 23, 2021

Closes: #678

Changes proposed in this PR:

  • Improving the search functionality. For example, previously if you searched for "data union" it would not return "DataUnion.App", now this has been corrected.

@vercel
Copy link

vercel bot commented Jul 23, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/oceanprotocol/market/FeenipgDNq7KLgQmXwcQu8gjv5Fc
✅ Preview: https://market-git-issue-678-improve-search-oceanprotocol.vercel.app

@claudiaHash
Copy link
Contributor

The new search works great! Although, I think there might be an issue when combining searching with filtering. For example, selecting ALGORTIHMS I got the following results:

Screenshot from 2021-07-26 12-51-04

@mihaisc
Copy link
Contributor

mihaisc commented Jul 26, 2021

The issue we still add in query service.attributes.main.type:algorithm, i saw this in the multinetwork branch. In this case is incorrect and in multi is not optimal ( at this point the difference is minimal ). The correct way is to add a term

 {
       "term":{
               "service.attributes.main.type": "algorithm"
          }
}

@jamiehewitt15 jamiehewitt15 self-assigned this Jul 28, 2021
@jamiehewitt15
Copy link
Contributor Author

jamiehewitt15 commented Jul 28, 2021

@claudiaHash Thank you for pointing that out, I have fixed the filters now.

@mihaisc I used match instead of term as that's the recommendation of the documentation when using text fields:

Avoid using the term query for text fields.

By default, Elasticsearch changes the values of text fields as part of analysis. This can make finding exact matches for text field values difficult.

To search text field values, use the match query instead.

https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-term-query.html

Copy link
Contributor

@kremalicious kremalicious left a comment

Choose a reason for hiding this comment

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

perfect functionality-wise. Crazy how bad our search was before :-)

@jamiehewitt15 jamiehewitt15 changed the title Issue 678 improve search Improve search Jul 28, 2021
Copy link
Member

@KatunaNorbert KatunaNorbert left a comment

Choose a reason for hiding this comment

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

looks good!

@kremalicious
Copy link
Contributor

right, the merge conflict. @jamiehewitt15 could you take another look at this please? As we now need to combine your changes with the query for multiple chainIds we now have in main and you're the elastic search expert now 😄

Screen Shot 2021-07-30 at 14 38 18

@mihaisc
Copy link
Contributor

mihaisc commented Aug 2, 2021

Ah yes, @jamiehewitt15 while merging could also change the chain id query to use match/tern in the boolean query (like we do with type and isPurgatory), and not leave it as a query string. 😁

@kremalicious kremalicious changed the title Improve search Improve search queries Aug 2, 2021
@kremalicious kremalicious self-requested a review August 2, 2021 10:30
Copy link
Contributor

@kremalicious kremalicious left a comment

Choose a reason for hiding this comment

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

as mentioned, merge main and adapt query. Then we should all test it again

@qlty-cloud-legacy
Copy link

Code Climate has analyzed commit 8a096a5 and detected 0 issues on this pull request.

View more on Code Climate.

@kremalicious kremalicious self-requested a review August 2, 2021 13:20
Copy link
Contributor

@kremalicious kremalicious left a comment

Choose a reason for hiding this comment

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

works flawlessly for me, now also with combinations of different networks selected and such, amazing improvement overall!

@jamiehewitt15 jamiehewitt15 merged commit 58781ff into main Aug 2, 2021
@jamiehewitt15 jamiehewitt15 deleted the issue-678-improve-search branch August 2, 2021 14:04
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.

Improve search (even more)

5 participants