Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Faster version of transWhere #623

Merged
merged 3 commits into from
Nov 16, 2020
Merged

Faster version of transWhere #623

merged 3 commits into from
Nov 16, 2020

Conversation

acasar
Copy link
Contributor

@acasar acasar commented Nov 13, 2020

This PR is an alternative version of #586

Instead of creating a separate method that has a different behavior to the original implementation, I managed to fix transWhere itself by utilizing 2 queries instead of the slow orWhere clause. This implementation also preserves the fallback functionality.

I made some tests on my project (originally discussed here: #581) - now I have 2 queries that execute in ~ 5 ms instead of a single 60 second query. The improvement is significant.

Comments are welcome.

This PR is an alternative version of rainlab#586

Instead of creating a separate method that has a different behavior to the original implementation, I managed to fix ``transWhere`` itself by utilizing 2 queries instead of the slow orWhere clause. This implementation also preserves the fallback functionality.

I made some tests on my project (originally discussed here: rainlab#581) - now I have 2 queries that execute in ~ 5 ms instead of a single 60 second query. The improvement is significant.

Comments are welcome.
@mjauvin
Copy link
Contributor

mjauvin commented Nov 13, 2020

I really like this implementation @acasar !

@mjauvin
Copy link
Contributor

mjauvin commented Nov 13, 2020

From initial tests I have been doing, it behaves the way I would expect it to.

Can you guys @mariavilaro @mterman @Eoler @aurelien-roy test this to see if you get the same performance improvements as @acasar and report back with numbers?

Thanks.

@mjauvin
Copy link
Contributor

mjauvin commented Nov 14, 2020

I did some tests, created 50,000 records for one of my models, and added a translation for those 50,000 records;

  • Without this PR, it took 3.39s to complete a transWhere().
  • With this PR, it took 54ms

So I don't have the kind of improvement I heard of, but it's still a significant (62X) improvement.

Good work!

@LukeTowers
Copy link
Contributor

@mjauvin I'm happy with this if you are. Great work @acasar!

@mjauvin mjauvin added this to the 1.7.4 milestone Nov 15, 2020
@mjauvin mjauvin linked an issue Nov 15, 2020 that may be closed by this pull request
@mjauvin mjauvin self-assigned this Nov 15, 2020
@mjauvin mjauvin changed the title Alternative version of faster transWhere Faster version of transWhere Nov 16, 2020
@mjauvin mjauvin merged commit 508c42c into rainlab:master Nov 16, 2020
@Samuell1
Copy link
Member

Samuell1 commented Mar 22, 2021

@acasar Hey, not sure why but this version of transWhere broke searching for slug for random slugs when i rolled it back to older version with leftJoin it works as expected.

After little bit of testing looks like its when multiple indexes even different models are with same value not sure why this happens.

@iboved
Copy link

iboved commented Jul 1, 2022

This version of transWhere returns the result in the default locale if the value does not exist in the desired locale. For example:

$page = Page::transWhere('slug', 'hello', 'de')->first();

I don't have slug = 'hello' in DE locale. Slug in DE locale is empty. But I have this slug in the default EN locale. And this query returns me Page where slug = 'hello' in default EN locale instead of null.

As a solution you can add one more parameter to scopeTransWhere function - noFallbackLocale which defaults to false.

@daftspunk
Copy link
Member

It might be worthwhile to keep the old implementation alongside the new one. What to call it?

@acasar acasar deleted the patch-2 branch July 2, 2022 08:10
@acasar
Copy link
Contributor Author

acasar commented Jul 2, 2022

I think what @iboved is referring to is to have an option to add a whereTrans clause without a default fallback. Bringing back the original implementation would not solve that because it worked the same way - if translated slug was not found, it checked the default slug with an orWhere clause.

Another parameter would need to be introduced to enable that behavior. In few cases where I needed that I created my own scope and queried translated table manually. But it may be nice to have this option by default. This is the scope I use:

public function scopeTransWhereNoFallback($query, $index, $value, $locale = null, $operator = '=')
{
    $translateIndexes = \Db::table('rainlab_translate_indexes')
        ->where('rainlab_translate_indexes.model_type', '=', $this->getMorphClass())
        ->where('rainlab_translate_indexes.locale', '=', $locale ?: app()->getLocale())
        ->where('rainlab_translate_indexes.item', $index)
        ->where('rainlab_translate_indexes.value', $operator, $value)
        ->pluck('model_id');

    return $query->whereIn($this->getQualifiedKeyName(), $translateIndexes);
}

@daftspunk
Copy link
Member

Got it. Something added for this here: fdec343

@iboved
Copy link

iboved commented Jul 5, 2022

Thanks, guys, for the quick response and solution!
Did I understand correctly that the scopeTransWhereNoFallback method works only with non-default locales? It searches only in the rainlab_translate_indexes table (while the value from the default locale is stored in the original model table). This means that we can not use this method to search value in the default locale. For default locale we should use scopeTransWhere. Is it correct behavior?

@acasar
Copy link
Contributor Author

acasar commented Jul 5, 2022

Actually you should just use a normal where for the default locale, since you don't want to query translatable data in this case.

$trans = Translator::getInstance();

if($trans->getLocale() === $trans->getDefaultLocale()) {
    $post = Post::where('slug', $slug)->first();
} else {
    $post = Post::transWhereNoFallback('slug', $slug)->first();
}

But I agree that this condition should probably be handled within transWhereNoFallback method. I made a PR with a suggested fix here: #685 Although, maybe this needs some more discussion....

Currently we have the following situation. Imagine we have 2 Posts:

Locale Post 1 (slug) Post 2 (slug)
en first-post second-post
sl prvi-clanek null

If we use the following where conditions in en locale, we get these results:

Condition (en) first-post second-post prvi-clanek
transWhere Post 1 Post 2 No result
transWhereNoFallback Post 1* Post 2* No result

* No result without this PR: #685

If we use the following where conditions in sl locale, we get these results:

Condition (sl) first-post second-post prvi-clanek
transWhere Post 1 Post 2 Post 1
transWhereNoFallback No result No result Post 1

But I think what we actually want when we run the query in sl locale is this:

Condition (sl) first-post second-post prvi-clanek
Ideal behavior No result Post 2 Post 1

So currently no implementation of transWhere is perfect. The old implementation worked in the same way as transWhere works now, so it had the same issue. But I don't know, maybe this is not that critical, since the only "side effect" of using transWhere in a foreign locale is that it returns a result in two cases - when using the original slug and also when using the translated slug. We never really had any issues with that, since links with the original slugs do not appear anywhere on the site and are therefore not indexed by Google. We also set canonical links to all our pages, which further minimizes the issue.

Potential problem could arise if one of the translated slugs matches one of the default slugs, but it's not for the same post. We never had a case like that, but maybe that's exactly the reason why @iboved opened this issue in the first place?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

transWhere is very slow
6 participants