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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new topological sort implementation #10592

Merged
merged 1 commit into from
May 8, 2023

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Mar 22, 2023

This is the first chunk to break #10547 into smaller PRs suitable for reviewing. It adds a new topological sort implementation.

Background

Topological sort is an algorithm that sorts the vertices of a directed acyclic graph (DAG) in a linear order such that for every directed edge from vertex A to vertex B, vertex A comes before vertex B in the ordering. This ordering is called a topological order.

Ultimately (beyond the scope of this PR), in the ORM we'll need this to find an order in which we can insert new entities into the database. When one entity needs to refer to another one by means of a foreign key, the referred-to entity must be inserted before the referring entity. Deleting entities is similar.

A topological sorting can be obtained by running a depth first search (DFS) on the graph. The order in which the DFS finishes on the vertices is a topological order. The DFS is possible iif there are no cycles in the graph. When there are cycles, the DFS will find them.

For more information about topological sorting, as well as a description of an DFS-based topological sorting algorithm, see https://en.wikipedia.org/wiki/Topological_sorting.

Current situation

There is a DFS-based topological sorting implemented in the CommitOrderCalculator. This implementation has two kinks:

  1. It does not detect cycles

When there is a cycle in the graph, we need to know about it. Ultimately, this means we will not be able to insert entities into the database in any order that allows all foreign keys constraints to be satisfied.

If you look at CommitOrderCalculator, you'll see that there is no code dealing with this situation.

  1. It has an obscure concept of edge "weights"

To me, it is not clear how those are supposed to work. The weights are related to whether a foreign key is nullable or not, but can (could) be arbitrary integers. An edge will be ignored if it has a higher (lower) weight than another, already processed edge... 馃し馃徎?

Suggested change

In fact, when inserting entities into the database, we have two kinds of foreign key relationships: Those that are nullable, and those that are not.

Non-nullable foreign keys are hard requirements: Referred-to entities must be inserted first, no matter what. These are "non-optional" edges in the dependency graph.

Nullable foreign keys can be set to NULL when first inserting an entity, and then coming back and updating the foreign key value after the referred-to (related) entity has been inserted into the database. This is already implemented in \Doctrine\ORM\UnitOfWork::scheduleExtraUpdate, at the expense of performing one extra UPDATE query after all the INSERTs have been processed. These edges are "optional".

When finding a cycle that consists of non-optional edges only, treat it as a failure. We won't be able to insert entities with a circular dependency when all foreign keys are non-NULLable.

When a cycle contains at least one optional edge, we can use it to break the cycle: Use backtracking to go back to the point before traversing the last optional edge. This omits the edge from the topological sort order, but will cost one extra UPDATE later on.

To make the transition easier, the new implementation is provided as a separate class, which is marked as @internal.

Outlook

Backtracking to the last optional edge is the simplest solution for now. In general, it might be better to find another (optional) edge that would also break the cycle, if that edge is also part of other cycles.

Remember, every optional edge skipped costs an extra UPDATE query later on. The underlying problem is known as the "minimum feedback arc set" problem, and is not easily/efficiently solvable. Thus, for the time being, picking the nearest optional edge seems to be reasonable.

Extra bonus

This is what the DALL路E AI thinks it looks like when you have a directed graph with cycles, and some wizardry finds a solution to break those.

DALL路E 2023-03-22 22 53 55

@mpdude mpdude force-pushed the new-topo-sort branch 2 times, most recently from 8e3d0d8 to 8b5f8ee Compare March 23, 2023 08:20
@greg0ire
Copy link
Member

Git says the concept of "weight" was introduced in #1570
@guilhermeblanco hi! Could you maybe shed some light on how they work?

This is the first chunk to break doctrine#10547 into smaller PRs suitable for reviewing. It adds a new topological sort implementation.

 #### Background

Topological sort is an algorithm that sorts the vertices of a directed acyclic graph (DAG) in a linear order such that for every directed edge from vertex A to vertex B, vertex A comes before vertex B in the ordering. This ordering is called a topological order.

Ultimately (beyond the scope of this PR), in the ORM we'll need this to find an order in which we can insert new entities into the database. When one entity needs to refer to another one by means of a foreign key, the referred-to entity must be inserted before the referring entity. Deleting entities is similar.

A topological sorting can be obtained by running a depth first search (DFS) on the graph. The order in which the DFS finishes on the vertices is a topological order. The DFS is possible iif there are no cycles in the graph. When there are cycles, the DFS will find them.

For more information about topological sorting, as well as a description of an DFS-based topological sorting algorithm, see https://en.wikipedia.org/wiki/Topological_sorting.

 #### Current situation

There is a DFS-based topological sorting implemented in the `CommitOrderCalculator`. This implementation has two kinks:

1. It does not detect cycles

When there is a cycle in the DAG that cannot be resolved, we need to know about it. Ultimately, this means we will not be able to insert entities into the database in any order that allows all foreign keys constraints to be satisfied.

If you look at `CommitOrderCalculator`, you'll see that there is no code dealing with this situation.

2. It has an obscure concept of edge "weights"

To me, it is not clear how those are supposed to work. The weights are related to whether a foreign key is nullable or not, but can (could) be arbitrary integers. An edge will be ignored if it has a higher (lower) weight than another, already processed edge... 馃し馃徎?

 #### Suggested change

In fact, when inserting entities into the database, we have two kinds of foreign key relationships: Those that are `nullable`, and those that are not.

Non-nullable foreign keys are hard requirements: Referred-to entities must be inserted first, no matter what. These are "non-optional" edges in the dependency graph.

Nullable foreign keys can be set to `NULL` when first inserting an entity, and then coming back and updating the foreign key value after the referred-to (related) entity has been inserted into the database. This is already implemented in `\Doctrine\ORM\UnitOfWork::scheduleExtraUpdate`, at the expense of performing one extra `UPDATE` query after all the `INSERT`s have been processed. These edges are "optional".

When finding a cycle that consists of non-optional edges only, treat it as a failure. We won't be able to insert entities with a circular dependency when all foreign keys are non-NULLable.

When a cycle contains at least one optional edge, we can use it to break the cycle: Use backtracking to go back to the point before traversing the last _optional_ edge. This omits the edge from the topological sort order, but will cost one extra UPDATE later on.

To make the transition easier, the new implementation is provided as a separate class, which is marked as `@internal`.

 #### Outlook

Backtracking to the last optional edge is the simplest solution for now. In general, it might be better to find _another_ (optional) edge that would also break the cycle, if that edge is also part of _other_ cycles.

Remember, every optional edge skipped costs an extra UPDATE query later on. The underlying problem is known as the "minimum feedback arc set" problem, and is not easily/efficiently solvable. Thus, for the time being, picking the nearest optional edge seems to be reasonable.
@mpdude
Copy link
Contributor Author

mpdude commented Apr 24, 2023

@derrabus I've added the @internal docbock and a comment explaining the use of array_reverse.

@mpdude
Copy link
Contributor Author

mpdude commented May 8, 2023

@derrabus Do you 馃憤 this? I need to get this merged into the temporary branch so I can continue working on #10547.

@derrabus derrabus merged commit 9ac063d into doctrine:entity-level-commit-order May 8, 2023
41 checks passed
@mpdude mpdude deleted the new-topo-sort branch May 8, 2023 11:32
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 8, 2023
This is the third step to break doctrine#10547 into smaller PRs suitable for reviewing. It uses the new topological sort implementation from doctrine#10592 and the refactoring from doctrine#10651 to compute the UoW's commit order for entity insertions not on the entity class level, but for single entities and their actual dependencies instead.

#### Current situation

`UnitOfWork::getCommitOrder()` would compute the entity sequence on the class level with the following code:

https://github.com/doctrine/orm/blob/70477d81e96c0044ad6fd8c13c37b2270d082792/lib/Doctrine/ORM/UnitOfWork.php#L1310-L1325

#### Suggested change

* Instead of considering the classes of all entities that need to be inserted, updated or deleted, consider the new (inserted) entities only. We only need to find a sequence in situations where there are foreign key relationships between two _new_ entities.
* In the dependency graph, add edges for all to-one association target entities.
* Make edges "optional" when the association is nullable.

#### Test changes

I have not tried to fully understand the few changes necessary to fix the tests. My guess is that those are edge cases where the insert order changed and we need to consider this during clean-up.

Keep in mind that many of the functional tests we have assume that entities have IDs assigned in the order that they were added to the EntityManager. That does not change 鈥撀爏o the order of entities is generally stable, equal to the previous implementation.
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 8, 2023
This is the third step to break doctrine#10547 into smaller PRs suitable for reviewing. It uses the new topological sort implementation from doctrine#10592 and the refactoring from doctrine#10651 to compute the UoW's commit order for entity insertions not on the entity class level, but for single entities and their actual dependencies instead.

#### Current situation

`UnitOfWork::getCommitOrder()` would compute the entity sequence on the class level with the following code:

https://github.com/doctrine/orm/blob/70477d81e96c0044ad6fd8c13c37b2270d082792/lib/Doctrine/ORM/UnitOfWork.php#L1310-L1325

#### Suggested change

* Instead of considering the classes of all entities that need to be inserted, updated or deleted, consider the new (inserted) entities only. We only need to find a sequence in situations where there are foreign key relationships between two _new_ entities.
* In the dependency graph, add edges for all to-one association target entities.
* Make edges "optional" when the association is nullable.

#### Test changes

I have not tried to fully understand the few changes necessary to fix the tests. My guess is that those are edge cases where the insert order changed and we need to consider this during clean-up.

Keep in mind that many of the functional tests we have assume that entities have IDs assigned in the order that they were added to the EntityManager. That does not change 鈥撀爏o the order of entities is generally stable, equal to the previous implementation.
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 9, 2023
This is the third step to break doctrine#10547 into smaller PRs suitable for reviewing. It uses the new topological sort implementation from doctrine#10592 and the refactoring from doctrine#10651 to compute the UoW's commit order for entity insertions not on the entity class level, but for single entities and their actual dependencies instead.

#### Current situation

`UnitOfWork::getCommitOrder()` would compute the entity sequence on the class level with the following code:

https://github.com/doctrine/orm/blob/70477d81e96c0044ad6fd8c13c37b2270d082792/lib/Doctrine/ORM/UnitOfWork.php#L1310-L1325

#### Suggested change

* Instead of considering the classes of all entities that need to be inserted, updated or deleted, consider the new (inserted) entities only. We only need to find a sequence in situations where there are foreign key relationships between two _new_ entities.
* In the dependency graph, add edges for all to-one association target entities.
* Make edges "optional" when the association is nullable.

#### Test changes

I have not tried to fully understand the few changes necessary to fix the tests. My guess is that those are edge cases where the insert order changed and we need to consider this during clean-up.

Keep in mind that many of the functional tests we have assume that entities have IDs assigned in the order that they were added to the EntityManager. That does not change 鈥撀爏o the order of entities is generally stable, equal to the previous implementation.
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 9, 2023
This is the third step to break doctrine#10547 into smaller PRs suitable for reviewing. It uses the new topological sort implementation from doctrine#10592 and the refactoring from doctrine#10651 to compute the UoW's commit order for entity insertions not on the entity class level, but for single entities and their actual dependencies instead.

#### Current situation

`UnitOfWork::getCommitOrder()` would compute the entity sequence on the class level with the following code:

https://github.com/doctrine/orm/blob/70477d81e96c0044ad6fd8c13c37b2270d082792/lib/Doctrine/ORM/UnitOfWork.php#L1310-L1325

#### Suggested change

* Instead of considering the classes of all entities that need to be inserted, updated or deleted, consider the new (inserted) entities only. We only need to find a sequence in situations where there are foreign key relationships between two _new_ entities.
* In the dependency graph, add edges for all to-one association target entities.
* Make edges "optional" when the association is nullable.

#### Test changes

I have not tried to fully understand the few changes necessary to fix the tests. My guess is that those are edge cases where the insert order changed and we need to consider this during clean-up.

Keep in mind that many of the functional tests we have assume that entities have IDs assigned in the order that they were added to the EntityManager. That does not change 鈥撀爏o the order of entities is generally stable, equal to the previous implementation.
mpdude added a commit to mpdude/doctrine2 that referenced this pull request May 31, 2023
This is the third step to break doctrine#10547 into smaller PRs suitable for reviewing. It uses the new topological sort implementation from doctrine#10592 and the refactoring from doctrine#10651 to compute the UoW's commit order for entity insertions not on the entity class level, but for single entities and their actual dependencies instead.

 #### Current situation

`UnitOfWork::getCommitOrder()` would compute the entity sequence on the class level with the following code:

https://github.com/doctrine/orm/blob/70477d81e96c0044ad6fd8c13c37b2270d082792/lib/Doctrine/ORM/UnitOfWork.php#L1310-L1325

 #### Suggested change

* Instead of considering the classes of all entities that need to be inserted, updated or deleted, consider the new (inserted) entities only. We only need to find a sequence in situations where there are foreign key relationships between two _new_ entities.
* In the dependency graph, add edges for all to-one association target entities.
* Make edges "optional" when the association is nullable.

 #### Test changes

I have not tried to fully understand the few changes necessary to fix the tests. My guess is that those are edge cases where the insert order changed and we need to consider this during clean-up.

Keep in mind that many of the functional tests we have assume that entities have IDs assigned in the order that they were added to the EntityManager. That does not change 鈥撀爏o the order of entities is generally stable, equal to the previous implementation.
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.

None yet

3 participants