Skip to content

Conversation

@marcelklehr
Copy link
Member

No description provided.

@marcelklehr marcelklehr force-pushed the fix/migration-limit-in-subquery branch from ad00d9c to 23ca9ca Compare September 1, 2025 08:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This migration removes a subquery from a database update operation by replacing it with separate query execution and parameter binding. The change eliminates the use of createFunction() with a nested SQL query in favor of fetching IDs first and then using them as parameters.

  • Replaced subquery pattern with parameter-based approach for better database compatibility
  • Added explicit result handling with cursor management
  • Introduced PDO import for fetch operations

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

$query->update('recognize_face_detections')
->set('face_vector', 'vector')
->where($query->expr()->in('id', $query->createFunction('(' . $select->getSQL() .')')));
->where($query->expr()->in('id', $query->createParameter('ids')));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you be able to update all with a single query?

Suggested change
->where($query->expr()->in('id', $query->createParameter('ids')));
->where($select->expr()->isNull('face_vector'));
$query->executeStatement()

if you are not doing any data manipulation that should work just fine.
No previous select or anything needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't you be able to update all with a single query?

Doesn't seem work for large tables, sadly. See #1357

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put a transaction around it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But anyway, should also be good like this, but it will run much longer.

But also in this version a transaction should speed up the update I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh, the issue in #1357 is due to a long running transaction, wouldn't wrapping the current state in this branch in a transaction introduce the issue of a long running transaction again?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can close the transaction after each chunk.
THe idea is to reduce the number of writes

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@marcelklehr marcelklehr force-pushed the fix/migration-limit-in-subquery branch from 23ca9ca to 6852ac1 Compare September 1, 2025 09:13
@marcelklehr marcelklehr closed this Sep 1, 2025
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.

3 participants