Optimize joins and table selection in SQL entity query implementation

I like the general idea to be honest. Are we sure we execute the uuid call that often on runtime?

Watching this with interest as it might make it easier to support querying for the latest revisions of referenced entities in #2864995: Allow entity query to query the latest revision.

We have a good example for a slow entity query generated by the patch from #2837707-30: EntityChangedConstraintValidator should be retrieving the latest changed time through an entity query instead by loading the unchanged entity, which was demonstrated by @kfritsche in #2837707-35: EntityChangedConstraintValidator should be retrieving the latest changed time through an entity query instead by loading the unchanged entity:

We have 8 languages and around 4000 entities (16.000 rows in the table - so not all is translated to all languages yet)

Running this query takes 1 minute.

Rerolling this for 8.9.x to troubleshoot some slow queries.

Related to https://www.drupal.org/project/drupal/issues/3088098? Is there any overlap?

This was discussed as part of the Bug Smash Initiative along with #3006315: Entity sorting and filtering can be extremely slow and #3088098: Duplicated joins in entity query, the decision was made to consolidate all three into the oldest issue (this one).

Credit will be transferred to this issue shortly.

Of note is that was considerable discussion and several patches on #3088098: Duplicated joins in entity query that may be useful additions to the work done here.

@darvanen asked in slack for credit to be transferred. So here it is.

Hi, I tried the D8 patch and found an issue, I wonder if its something we have wrong in the entity configuration or the patch introduces a bug...

 $query = \Drupal::entityTypeManager()
      ->getStorage('ENTITY_A')->getQuery()
      ->condition('ENTITY_REFERENCE_A.entity:ENTITY_B.ENTITY_FIELD_X', $x)
      ->condition('ENTITY_REFERENCE_A.entity:ENTITY_B.ENTITY_FIELD_Y', $y);

ENTITY_A has an entity reference field (ENTITY_REFERENCE_A) to ENTITY_B, the query that this produces (ignoring the conditions) to join the tables is this:

SELECT base_table.id AS id, base_table.id AS base_table_id
FROM
ENTITY_A base_table
INNER JOIN ENTITY_B ON ENTITY_B.id = base_table.id
WHERE ENTITY_B.x = 'x' 

Note the INNER JOIN is incorrect:

INNER JOIN ENTITY_B ON ENTITY_B.id = base_table.id

should actually be:

INNER JOIN ENTITY_B ON ENTITY_B.id = base_table.ENTITY_REFERENCE_A

Update, the problem is introduced in the patch https://www.drupal.org/files/issues/2020-12-08/2875033-16.patch in this part of the code:

@@ -297,7 +301,7 @@ public function addField($field, $type, $langcode) {
           $entity_type = $this->entityTypeManager->getActiveDefinition($entity_type_id);
           $field_storage_definitions = $this->entityFieldManager->getActiveFieldStorageDefinitions($entity_type_id);
           // Add the new entity base table using the table and sql column.
-          $base_table = $this->addNextBaseTable($entity_type, $table, $sql_column, $field_storage);
+          //$base_table = $this->addNextBaseTable($entity_type, $table, $sql_column, $field_storage);
           $propertyDefinitions = [];
           $key++;
           $index_prefix .= "$next_index_prefix.";

The other changes in the patch reduce the joins on the base table, but this causes bad joins for the entity references

UPDATE

Investigating further I can see why. This line updates the relative "Base table" for the next join, in the next for loop the following method is called:

        $table = $this->ensureEntityTable($index_prefix, $sql_column, $type, $langcode, $base_table, $entity_id_field, $entity_tables);

Looking at this method, this is what it does:

          $this->entityTables[$key] = $this->addJoin($type, $table, "%alias.$id_field = $base_table.$id_field", $langcode);

As you can see, there is only 1 parameter for the field id and it uses the same for both. Either this method needs 2 inputs (field id for each table) or we should be calling another method similar to ensureFieldTable().

Interesting to note in addField() is that there is a variable called "$propertyDefinitions" and another called "$property_definitions". Either this is a bug from renaming and we have left the old variable in place or one should be renamed

COMMENTED DELETED

I've built on the approach in #3088098: Duplicated joins in entity query and added some code to explicitly prevent a repeated join onto the base table - this seems to be because of the discrepancy between "base_table" and the actual name of the base table being used as the alias.

Curious how this handles non-sql based entities. Thinking of external entities and similar modules if this would limit that functionality.

I would also say that the logical next step would be to make views based of entity queries. Then this logic would only need to be fixed once.

Does this still allow joins to the base table with the joined table having a different alias? Like this:

SELECT
	column1,
	column2,
	column3,
        ...
FROM
	table1 A
INNER JOIN table1 B ON B.column1 = A.column2;

patch 16 is breaking https://www.drupal.org/project/drupal/issues/2875033#comment-13931513

one and the same query before and after patch (in context of json API)

SELECT base_table.vid AS vid,
       base_table.nid AS nid
FROM node base_table
LEFT JOIN node__body node__body ON node__body.entity_id = base_table.nid
LEFT JOIN node_field_data node_field_data ON node_field_data.nid = base_table.nid
INNER JOIN node__field_taxonomy_country node__field_taxonomy_country ON node__field_taxonomy_country.entity_id = base_table.nid
LEFT OUTER JOIN taxonomy_term_data taxonomy_term_data ON taxonomy_term_data.tid = node__field_taxonomy_country.field_taxonomy_country_target_id
INNER JOIN taxonomy_term_field_data taxonomy_term_field_data ON taxonomy_term_field_data.tid = taxonomy_term_data.tid
INNER JOIN node_field_data node_field_data_2 ON node_field_data_2.nid = base_table.nid
WHERE (((node__body.body_value LIKE '%agent%' ESCAPE '\\')
        OR (node_field_data.title LIKE '%agent%' ESCAPE '\\'))
       AND (taxonomy_term_field_data.name IN ('Poland',
                                                  'Polska')))
  AND (node_field_data_2.type = 'job_offering')
GROUP BY base_table.vid,
         base_table.nid
LIMIT 11
OFFSET 0

SELECT base_table.vid AS vid,
       base_table.nid AS nid
FROM node_field_data base_table
LEFT JOIN node__body node__body ON node__body.entity_id = base_table.nid
INNER JOIN node__field_taxonomy_country node__field_taxonomy_country ON node__field_taxonomy_country.entity_id = base_table.nid
INNER JOIN taxonomy_term_field_data taxonomy_term_field_data ON taxonomy_term_field_data.tid = base_table.tid
WHERE (((node__body.body_value LIKE :db_condition_placeholder_0 ESCAPE '\\')
        OR (base_table.title LIKE :db_condition_placeholder_1 ESCAPE '\\'))
       AND (taxonomy_term_field_data.name IN (:db_condition_placeholder_2,
                                              :db_condition_placeholder_3)))
  AND (base_table.type = :db_condition_placeholder_4)
GROUP BY base_table.vid,
         base_table.nid
LIMIT 11
OFFSET 0;

So after (2nd) won`t work with SQL error "tid not found on base table"

Seems there are still some open questions to answer before review.

#39 and #41 should be answered (added to remaining tasks)

Faced this issue with "single-table" entity type. Entity query joined base table to itself which caused bad performance. Patch #16 works well on Drupal 10.0.

Patch #16 works well on Drupal 10.0.

Actually it does not. EFQ with entity references produces wrong SQL join. See comment #30.

We have been using the mr in #37 for a while, but there are some issues with it: when using multiple conditions on the same column in an entity-query, the same table is joined several times.

I've fixed this with another if-statement in the patch that checks if the table is already joined (with the same type).

ps. this patch applies to drupal 11, not cleanly to 10 (because of the comment above the piece code)

@spadxiii Please make the change to the merge request, rather than submitting a patch.

I seem to have attached the wrong patch. Here's the correct one, that works.

@solideogloria the mr is quite old and needs to be rebased :\

and when I push, I get an error that: remote: You are not allowed to push code to this project.
So I cannot update the mr.

You have to click the "Get Push Access" button at the top of this page.

It might be easier to open a new MR into 11.x.

https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

Hi,

I have tried to create MR for the changes mentioned in patch #48 but was unable to do so because the MR points to branch 9.5.x instead of 11.x. Also, I have tried to create a new branch from 11.x but getting the below error:

Thanks & Regards,
Mrinalini

@mrinalini9 This should be helpful for you: Rebase to a new base branch

To run the tests, I created an MR of patch #48.

Thanks for this !

Attach is the patch from #48 (2875033-46.patch) rerolled for Drupal 10.3.x and 10.4.x

The changes need to be applied to the merge request.

patch #61 is failing for my colleague when filtering with jsonapi on the value of a referenced entity referenced by the main entity.

It's writing the WHERE clause such that it's filtering the main entity to the node ID of the referenced entity.

example:

• main entity house node.
• house references a city node
• city references a state node

if I filter houses in jsonapi by state, the SQL where clause is filtering the house node ID by the desired state node ID.

This issue and #3022864: \Drupal\Core\Entity\Query\Sql\Tables causes extremely poor performance when using MariaDB and filtering on multiple relationships in JSON:API IMO needs to be consolidated.

Hi all.
First of all, I want to thank everyone who is working on this task. It solves the performance problem for big data entities in certain cases.

But I also found a possible way to make it better for some scenarios.
We can remove `$type === 'INNER'` from the condition.
If the table is the same, then it doesn't matter which type of join is used. Anyway, the same table will be used.
Removing this condition can be useful for big data queries with base fields of the entity, which are stored in the same table if the data table is the same as the base table for an entity.

For example,
- we have a `custom_entity`
- we send a simple entity query to get IDs sorted by uuid
- the default query will be

SELECT base_table.id AS id, base_table.id AS base_table_id, custom_entity.uuid AS uuid
FROM
custom_entity base_table
LEFT JOIN custom_entity custom_entity ON custom_entity.id = base_table.id
ORDER BY custom_entity.uuid ASC
LIMIT 20 OFFSET 0

What is the problem? If custom_entity is a big data entity, then we are trying to join big data to big data, which will take much more time than without 'join'. This impacts performance a lot

If we remove `$type === 'INNER'` part of the condition, the issue will be solved, because the query will be generated like

SELECT base_table.id AS id, base_table.id AS base_table_id, base_table.uuid AS uuid
FROM
custom_entity base_table
ORDER BY base_table.uuid ASC
LIMIT 20 OFFSET 0

I added a hidden patch with this fix.

P.S. Please let me know if my opinion is correct or if it has obvious flaws in the context of Drupal core

I've attempted to work on this a little and produced the following based on 11.x:
https://git.drupalcode.org/issue/drupal-2875033/-/commit/41ac87795f50465...

(Up front disclosure - I tested this as a patch on 10.4.x and this is just a cherry pick for analysis)

Here is an outline:
* For testing, a clone of a Query will now properly clone the conditions and update the attached query on the Conditions. Happy to take this out of this issue into another if it's desired. Essentially at the moment __toString on a Query is going to work with Conditions that still refer to the original Query. It causes problems in debugging mostly.
* getTables on Query was creating a new tables instance, not returning the existing property value. This meant tables added via separate Condition instances (such as nested in an AND or an OR) were not deduplicated - they were effectively creating new joins for every branch. This change keeps getTables for BC but it should be now unused, and it introduces getSqlTables that returns the same object after creating it so all conditions share the same instance, preventing attaching the same table twice
* Specifying different langcode in conditions for a shared table field previously could cause issues as it would reuse the same table join even though it would refer to a different langcode - now it joins separately for each langcode
* When following a reference field, the tables were never shared at all by addNextBaseTable, and this introduces a nextBaseTables property to track and reuse these
* When there is a data table, we don't just forcefully set simple_query to FALSE. Warrants some eyes and testing but it feels this is unnecessary as the table joining code that attaches the data table already sets it to FALSE at the point the table might get attached. In this change it actually only sets simple_query to FALSE if the join of a data table is not going to use a langcode. If a langcode is used, then the data table will provide a single row, simplifying the query.
* Added ability to specify langcode as "__default__" to make it join the data table on default_langcode=1 with simple_query remaining. For me this looks to massively improve query performance and allow us to add indexes in several places on the data table for massive gain. The API here might warrant discussion.

A note worth mentioning is that if a table joins as INNER JOIN and the next request is for a LEFT JOIN it will not change it. It will remain as INNER. This is how it was previously when deduplicating of table join was working. This seems fine to me as if it's INNER there's a restriction so loosening it serves no purpose. For the inverse, where it is initially added as LEFT or LEFT OUTER, it also won't change it to INNER if a request comes in. It seems fine to me this as I think from what I can see every join that uses INNER will have a WHERE addition anyway, so the fact it's LEFT doesn't make much difference. It could be "nice" to clean it up to INNER but then you're modifying already added tables in the query and it feels the API change stretches too far.

Generally it looks like the improvements from this experiment are:
* Tables are no longer duplicated in all cases (from what I can see)
* Where multiple shared data table fields are used in conditions, indexes on the data table can now get properly used as they use the same table join, and the optimiser can kick in
* Grouping is now completely avoided for basic queries that don't involve the shared data table
* Sorting by a shared data field that is untranslated can also be massively sped up with a quick index addition and specifying the langcode "__default__" as this ensures it keeps simple query and works on the default langcode, which inherently will have the untranslated value for that field. For me personally this is what I was trying to get to to resolve some slow queries.

Would be great to get some feedback while I do some further testing and look at adding some tests etc.

I've been testing MR 13291. Yet to add some tests.
It's working great, and using a lot more indexes in complex queries.

I added an improvement to prevent UUID duplicating the base table.

The only bit not in MR 13291 is the data table vs base table but I am unsure if that's much a huge benefit. As long as all the conditions sit on the same table (data table) and it has indexes it's more an aesthetic. So I was concentrating more on performance when you have indexes on a data table or a field table with multiple properties and need to prevent it adding different tables per condition that then bypass your indexes.

In my case I have one complex entity query with MR 13291 now completing in milliseconds rather than seconds as it can now properly use the indexes on the data table.

Happy to discuss improvements and I hope to start looking at tests soon.

One issue I did detect with my MR 13291 though is that if you create a condition using the WRONG query it will now generate a corrupt query. This seems bad code anyway but I did begin to think if there should be an assertion somewhere, or perhaps remove the sql query from Condition constructor and assign it during compilation time instead.

Following breaks, but maybe rightfully so, but before the MR, it works due to the issue of duplicating joins (it uses wrong tables instance to check table joined but does the join still in the right query)

```
$query = \Drupal::entityTypeManager()->getStorage('node')
->accessCheck(TRUE)
->condition('field', 'value')
->execute();

$query = \Drupal::entityTypeManager()->getStorage('node')
->accessCheck(TRUE)
// $query here is from the first query, so the condition group has different query during compilation, and so different table list
->condition($query->orConditionGroup()->condition('field', 'value1')->condition('field', 'value2'))
->execute();
```

Be good to hear thoughts. Thanks

I've completed the patch, fixing tests.

However, it did throw up one issue, a really obscure thing I never knew was possible, but for which a test exists:
https://github.com/drupal/drupal/blob/11.2.5/core/tests/Drupal/KernelTes...

Essentially, if you have a multi-valued field, the following will match no entities (and is same in current release):

```
$query = \Drupal::entityTypeManager()->getStorage('entity')->getQuery();
$query->accessCheck(FALSE)
->condition('multi_value_field', 1)
->condition('multi_value_field', 2)
->execute();
```

However, the following, in the test, demonstrates fetching an entity that has both 1 and 2 in its value list:

```
$query = \Drupal::entityTypeManager()->getStorage('entity')->getQuery();
$query->accessCheck(FALSE)
->condition($query->andConditionGroup()->condition('multi_value_field', 1))
->condition($query->andConditionGroup()->condition('multi_value_field', 2))
->execute();
```

So in other words - this appears to be a (relatively unknown?) feature at the moment that allows you to do an AND across deltas of a multi-valued field. This in itself fights against the concept of simplifying tables to prevent duplication as this very thing relies upon it.

I did end up making it work but it's quite ugly and I need to check if this kills the performance gains I had with the previous patch that stopped the above working. Worth a review and some discussion though.

I've updated the Issue summary with current status and better more complex examples that reflect the improvements.

Also added a section on `andConditionGroup` strange behaviour for discussion.

The `andConditionGroup` with a multi-value (cardinality > 1) field is a special case - it needs to join the tables again for each value check so that you can fetch an entity that has a property value X and a property value Y in the same entity. By putting `andConditionGroup()->condition('field', 'X')` as a condition alongside `andConditionGroup()->condition('field', 'Y')` this used to work - and is documented in a test as the way to do it...

In all honesty that `andConditionGroup` functionality seems obscure and the code would actually be hugely simplified if we could lose it and instead prefer people to do this: `->condition('field.*', 'X')->condition('field.*', 'Y')` - in other words use the `delta` but instead of referring to a specific value, say "any value" so it would read a bit better as "any field delta is X and any field delta is Y" which would ring true if there's an entity with both values on the multi-value field. It means we just detect this delta and join table anyway (deltas trigger their own separate joins). It removes the prefix changes trickling from condition and the need to detect multi-value - the user dictates they need it! And this way `andConditionGroup()` behaves as you'd expect. Also the `field.*` triggers you to search help "what is this syntax?" where at the moment it's not really clear to me `andConditionGroup` is working cross-value on a multi-value field!

The andConditionGroup behaviour in #71 sounds like a bug to me.

The way conditions are documented, shouldn't both versions of the code return the same thing?

> @trigger_error('Passing a Condition to \Drupal\Core\Entity\Query\Sql\Query::condition() that was generated by a different query is deprecated in drupal:12.0.0 and removed in drupal:13.0.0. See https://www.drupal.org/project/drupal/issues/2875033', E_USER_DEPRECATED);

Is this removing the behaviour that was added in https://www.drupal.org/node/2770421?

@joachim That's at the database abstraction layer. This is the entityQuery layer, which is one layer above. If you tried to add a subquery with entityQuery it would break - it never worked from what I can gather, currently it reports `Error Call to a member function getColumns() on false` from the Tables instance. If you check the signature for the `field` parameter (first parameter) of `condition()` it is `string|ConditionInterface` and an entityQuery is neither. If you think about it, entityQuery needs to find and locate tables, so each condition needs to be resolvable - if subquery were allowed it would have to "defer" resolving as it might refer to the outer query - so yeh not something I am sure is easy or even desired I guess.

The only supported parameter to `condition()` is a field name (`string`) or an aggregate `ConditionInterface` created via `andConditionGroup()` or `orConditionGroup`.

I'll update IS about the fact we do need to update the deprecation messages, I made them up to pass tests and CR might be needed if this is the right approach.

> The andConditionGroup behaviour in #71 sounds like a bug to me.
> The way conditions are documented, shouldn't both versions of the code return the same thing?

This is my thought too. Everything gets simpler if we drop this behaviour. But I just wanted to gather feedback as there was a test confirming it as desired. And indeed if we removed it there would be no way to find in a list of entities an entity that had both RED and GREEN in its colours multi-value field. You'd only be able to get entities that had RED in the list, entities that had GREEN in the list, or ones that had either or, you wouldn't be able to filter for ones that had both. But that's where I suggest using the `delta` syntax and using `*` or something as that makes it a bit more clearer and cleaner to do this kind of query (without needing the `andConditionGroup`) and also keeps the Tables/Query/Condition code simple as it already deals with duplicating tables based on `delta`.

Not confident on my own at this layer to make that decision - kind of needs a maintainer to feed in. I added tag for this (sorry if I'm not meant to be adding those tags.)

> Not confident on my own at this layer to make that decision - kind of needs a maintainer to feed in

This isn't the issue to fix it -- this is meant to be a refactoring & optimising issue.

As you've been working a lot with this code, you're in a good position to say whether we should fix that before or after this issue gets in.

@joachim Thanks for the feedback. I think the optimisation is not possible without either removing the `andConditionGroup` multi-value functionality or adding the extra complexity - and I already went the latter route to maintain BC so perhaps we just stick with this MR as it is then.

I'm happy to then provide some help and support in a follow up to rework the solution for the `andConditionGroup` piece, to simplify Entity Query functionality and allow for a migration path, e.g. one issue to add `field.*` syntax and a second issue to remove the `andConditionGroup` multi-value support.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

$query = \Drupal::entityTypeManager()->getStorage('entity')->getQuery();
$query->accessCheck(FALSE)
->condition('multi_value_field', 1)
->condition('multi_value_field', 2)

Is documented on the interface: "If two or more conditions have the same field names they apply to the same delta within that field". Tables::addField repeats this in an inline comment:

    // This variable ensures grouping works correctly. For example, given the
    // following conditions:
    // ->condition('tags', 2, '>')
    // ->condition('tags', 20, '<')
    // ->condition('node_reference.nid.entity.tags', 2)
    // The first two should use the same table

vs

$query = \Drupal::entityTypeManager()->getStorage('entity')->getQuery();
$query->accessCheck(FALSE)
->condition($query->andConditionGroup()->condition('multi_value_field', 1))
->condition($query->andConditionGroup()->condition('multi_value_field', 2))
->execute();

Well the condition now does not have the same field name instead they are separate condition groups. It's a degenerate case for condition groups but a valid use case nonetheless. There should be no special handling for this, it should just work because groups need to work on separate tables, don't they?

At least this is what I think but I've been known to be wrong. Which this issue shows because I created this mess originally.

@ghost-of-drupal-past

I'll get this issue rebased soon.

I have no issue with the documented element as it actually works very cleanly in the code. It's the `andConditionGroup` element NOT reusing the same table that is not really documented well and adds what I think is large complexity. It's also non-intuitive in my opinion and I do tend to think the delta parameter is largely unknown and unused and is kind of perfect for this scenario as you're not specifically "targeting different tables" you're "targeting different/same delta" so reusing that parameter would be way better and make things much easier to read. I highly doubt many Drupal devs will understand the andConditionGroup is not using the same table compared to the other query. Where with a delta of "*" they'll either know it uses different table (or even just think, examines all deltas), or at least explore the documentation of the parameter to know what it does. That's what I wanted to offer here for discussion - an improvement to the developer experience.

Not to throw AI around but also it'll help AI code generation significantly if we have delta well-documented and with a "*" to fulfil this purpose as it seems to make more logical sense as well as being self-documenting, which is where AI excels at its understanding.

I rebased this - just commit conflicts with JsonApiPerformanceTest. The performance test nicely shows the improvement here even for what should be a very simple query.

I also updated one code comment that I initially found hard to read, and added a strict comparison on a line that was already being changed, and changed the deprecation version to 11.4.0 (still for removal in 13.0.0).

On #71-#83 I think it's worth exploring whether there's a better syntax for this, but given there's test coverage for the existing behaviour and the current MR handles this case, I don't see a good reason to tackle it here - we can open a follow-up postponed on this issue to consider changing it (or if we decide not to, document why it works the way it currently does a bit more).

Also tentatively tagging for 11.4.0 release highlights because this would be a good sentence in the performance improvements section.

Postponed #3022864: \Drupal\Core\Entity\Query\Sql\Tables causes extremely poor performance when using MariaDB and filtering on multiple relationships in JSON:API on this issue - if there's something remaining there it will be easier to work on once this is in.

It's nice to see the changes in performance tests, but there's not much in the way of other test changes here. On the one hand it's good that all existing tests pass. This reminded me I've been meaning to open #3585125: Add EXPLAIN support to performance tests for a while so I did that and added this as a related issue, but we don't have that in core now so that kind of coverage shouldn't block a commit.

Not sure what else we could do for tests that cover the actual problem being fixed, maybe build a simple entity query like the uuid one, then extract the number of joins?

edit: also kicked off test runs on mariadb/sqlite/postgresql and they all look fine.

@berdir yeah I was looking at the join in the uuid query, it only joins to add default langcode and that's not necessary, but yes probably another follow-up, should be quite a small change after this one, already improved here.

Very minor feedback posted on MR. Noticed we don't have a CR to reference.

Opened a change record and updated the links. I struggled with the change record title, partly because I think we're deprecating something no-one would ever do on purpose anyway, so any better ideas very welcome.

Thanks everyone 🙏

@catch for the CR maybe something like this?

> Additionally, Drupal\Core\Entity\Query\Sql\Query::condition() will no longer accept a Condition object where that object was created using the factory methods (e.g. orConditionGroup()) on a different entity query object. This was never officially supported and prevented table join reuse, and will be explicitly disallowed from Drupal 13.0.0

I think the original is probably fine. This just gives an example of where you can do the previous odd behaviour in case someone is doing it without realising - and also gives the impact

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Did another review pass, and I think one change from the MR is out-of-scope here and should be moved into its own issue.

Also read the change record and it only covers the deprecation bits. Should we add something about how actual SQL queries have changed for the scenarios from the IS?

Added @amateescu's test coverage from the MR comment. We determined via experimentation that just the hunk of the MR related to LANGCODE_DEFAULT isn't enough to fix the bug that the test coverage covers, it's on top of the rest of the MR.

I thought I found another bug with cloning aggregate queries, but it wasn't caused by this MR, so I'll open a separate issue.

Added a new code suggestion, otherwise this looks ready to go after fixing the conflicts with #3308877: Add static cache for loadEntityByUuid function to store uuid-id pairs in memory.

Rebased and made the one suggested code change.

Nice, let's do this :)

Added some comments to the MR.

Applied a couple of suggestions, replied to the TableInterface::addField() point. Leaving needs work because I'm not sure what a good comment is for the langcode not-a-simple-query comment but agreed it would be good to add one.

I think that's the last round of feedback addressed again. Also did a rebase while here but that was all clean.

Looks ready again!

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Nope rebase is not correct. Issue that conflicted is #3585716: EntityQuery uses hard-coded langcode in revision-data joins, breaking translatable revisionable entities with custom langcode keys.

s/revisionTable/revisionDataTable/ was the only problem. Back to RTBC again.

Committed and pushed 702159f4ab2 to main and cfd9837f92f to 11.x and c143057061c to 11.4.x. Thanks!

Opened #3593233: Cloning an aggregate entity query shares its aggregate conditions with the original for a bug I found while reviewing this MR.

#2983639: Re-enable a bit of test coverage for Workspaces is also possible now, either thanks to this issue or from recent versions of mysql.

Thanks all! Super cool to see this now in place :)

Thanks for the work on this - we are eager to see how it impacts our performance!

Thanks all. I used this patch on my project on earlier stages and even did a small patch update, and it helped me a lot with a big data entity table. So I'm glad that it was already pushed.