Optimize EAV collections

[Sekiphp]

Some DB background

Overview

In cases when Magento collections are extended from Mage_Eav_Model_Entity_Collection_Abstract there are filtering by entity_type_id but for example for customer entity_type_id is always 1 (viz. screenshot)

But this conditions slow down multiple queries and I think, that at this time is abandoned using entity_type_id for most of queries.

[colinmollenhour]

I approve. Would be great to drop the column and index and save some db memory as well but this would likely break someone's code somewhere. I don't see an issue with this change though.

[colinmollenhour]

@Sekiphp Do you agree with the changes? Please update PR if so. Thanks!

[joshua-bn]

Bump @Sekiphp. Looks like a good suggestion from @colinmollenhour and pretty simple. Looks like a good change to implement.

[Sekiphp]

Your suggestion implemented @colinmollenhour ;-)

[colinmollenhour]

Looks good, thanks!

[Sekiphp]

Finally fixed @colinmollenhour @joshua-bn @tmotyl

[joshua-bn]

It's been a while since I looked at this so I might've lost some knowledge here.

For the entity tables like customer, catalog, etc, the entity_type_id will always be the same. For the eav_entity table (I don't think anyone ever uses that table) this does matter. The condition seems backwards then. If it is the default table, we should add the entity_type_id filter. If it is not then there's no need, right?

[colinmollenhour]

@joshua-bn is right, I had the logic backwards! Doh!

[Sekiphp]

It's been a while since I looked at this so I might've lost some knowledge here.

For the entity tables like customer, catalog, etc, the entity_type_id will always be the same. For the eav_entity table (I don't think anyone ever uses that table) this does matter. The condition seems backwards then. If it is the default table, we should add the entity_type_id filter. If it is not then there's no need, right?

So the right solution is?:

if ($this->getEntity()->getEntityType()->getEntityModel() == Mage_Eav_Model_Entity::DEFAULT_ENTITY_TABLE && $this->getEntity()->getTypeId()) { 
    $this->addAttributeToFilter('entity_type_id', $this->getEntity()->getTypeId());
}

@colinmollenhour I did some changes, because: $this->getEntity()->getEntityTable() returns table name and Mage_Eav_Model_Entity::DEFAULT_ENTITY_TABLE contains model path.

[joshua-bn]

@Sekiphp it should be the table, not the model.

I think this is what we're looking for:

    protected function _initSelect()
    {
        $this->getSelect()->from(['e' => $this->getEntity()->getEntityTable()]);

        if ($this->getEntity()->getEntityTable() === Mage_Eav_Model_Entity::DEFAULT_ENTITY_TABLE && $this->getEntity()->getTypeId()) {
            $this->addAttributeToFilter('entity_type_id', $this->getEntity()->getTypeId());
        }

        return $this;
    }

[joshua-bn]

btw, here's what M2 has for this https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Eav/Model/Entity/Collection/AbstractCollection.php#L217

[luigifab]

I'm not sure, what is the status of this PR?

[addison74]

With a little changes in code as requested we can merge this PR .

As Colin wrote, changes were implemented and/or he noticed his comment was wrong

[luigifab]

The SQL queries with our MariaDB 10.7:

MariaDB []> SELECT SQL_NO_CACHE COUNT(*) FROM customer_entity AS `e` WHERE (`e`.entity_type_id = '1');
+----------+
| COUNT(*) |
+----------+
|  1214517 |
+----------+
1 row in set (0.283 sec)

MariaDB []> SELECT SQL_NO_CACHE COUNT(*) FROM customer_entity AS `e` WHERE 1;
+----------+
| COUNT(*) |
+----------+
|  1214517 |
+----------+
1 row in set (0.245 sec)

I don't see problems at localhost with this change.

[kiatng]

Tested.

[luigifab]

So I tested the wrong version at localhost, but I didn't see any bad changes...

[fballiano]

this one is approved for merging, is everybody comfortable with this?

[kiatng]

I have this in production a month now. No issue so far.

[github-actions]

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 2946fcb. ± Comparison against base commit 712a3ee.