refactor: search optimizations #791
Conversation
…em/explorer.ark.io into refactor/search-optimizations
alfonsobries
requested review from
alexbarnsley,
faustbrian,
ItsANameToo and
samharperpittam
as code owners
alfonsobries
changed the title
…rk.io into refactor/search-optimizations
app/Models/Block.php
Outdated
| */ | ||
| public function scopeEmpty(Builder $query): Builder | ||
| { | ||
| return $query->whereNull('id'); |
There was a problem hiding this comment.
is this the preferred way to return an empty result? 🤔
There was a problem hiding this comment.
hmm I see now that you use these in else statements, but wouldn't it suffice to not create a query if the if does not hit? e.g. here https://github.com/ArkEcosystem/explorer.ark.io/pull/791/files#diff-b59c182bce7d848ab8acb5b96e78d56938d6ac1bf6128802f73eee834b288199R35
There was a problem hiding this comment.
@ItsANameToo the problem is that if the empty scope is not applied it will return all the blocks, so if my search query is, let's say, a random string like "asdfg" it incorrectly will show all the blocks since the resulted query will be select * from "blocks". What I'm doing is basically replacing the select * from "blocks" where lower(id) = "asdfg" that takes approx 4s (and we know will not return any result) for a simple select * from "blocks" where id = null that just takes a few milliseconds and also returns empty results
The problem is that I cannot just return an empty array, I still need a query builder instance since more conditions may apply, in other words, the query is just ensuring that we will not find any blocks by the given id by doing the simplest as fastest query I found. Since the following conditions (that may or not may apply) are orWhere still will be possible to find results, for example, if the query is a valid wallet username.
There was a problem hiding this comment.
@ItsANameToo regarding whether that's the preferred way I consider different solutions and I didn't find a better one. I'll revisit this PR in a moment to see if I come up with a cleaner alternative.
| } else { | ||
| // Forces empty results when it has a term but not possible | ||
| // block ID | ||
| $query->empty(); |
There was a problem hiding this comment.
why do we need to force empty here?
…rk.io into refactor/search-optimizations
|
@ItsANameToo didn't change the solution of empty scopes that are still open for discussion but refactored the empty scopes for something much cleaner. See: 15eb5e3 |
|
added a clickup task to reconsider those empty scopes https://app.clickup.com/t/m97mgv . Still feels like there's a better approach but at this time I cannot think of one. |
Summary
https://app.clickup.com/t/kz8hz2
Refactor the search-related classes to prevent unnecessary queries according to the search term for faster searches.
Checklist