Skip to content

Conversation

@mleczakm
Copy link
Contributor

@mleczakm mleczakm commented Oct 20, 2022

Change Log

Added

  • Mysql dialect for bulk update/insert/upsert

Fixed

Changed

Removed

Deprecated

Security


Description

Support for mysql db for loaders.

@norberttech
Copy link
Member

amazing @mleczakm ! Could you please take a look at failing jobs?

@norberttech
Copy link
Member

oh I know what is wrong, you need to add MySQL to the testsuite workflow file as well, it's not using docker-compose https://github.com/flow-php/flow/blob/1.x/.github/workflows/test-suite.yml#L74

@mleczakm

This comment was marked as outdated.

@github-actions github-actions bot added the ci/cd label Oct 29, 2022
@norberttech
Copy link
Member

some tests are failing now :(

@github-actions github-actions bot added size: M and removed size: L labels Nov 3, 2022
@mleczakm

This comment was marked as resolved.

@norberttech
Copy link
Member

not sure if I fully get the tests part, but let me explain my reasoning behind Bulk::insert and Bulk::upsert.

  • Bulk::insert - insert BulkData into given table, whenever records are already in that table, this operation should let us to update them (and here is where custom settings comes into play)
  • Bulk::update - update data in table taking values from BulkData

The reason why I decide to go with $insertOptions that takes optional keys is because I wasn't able to find common parts between PostgreSQL and MySQL so I decided that each engine can define it's own keys and make the optional.
There is also Dialect, that should prepare the query and check if all needed options are provided.

And about the tests, you don't need to replicate all PostgreSQL tests, it would be enough to just test all options supported by MySQL. As long as insert can do upserts and update is only about updates it should be fine.

*/
private function updateAllColumns(Columns $columns) : string
{
return \implode(
Copy link
Member

Choose a reason for hiding this comment

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

this looks to me like a logic that could be moved to Columns object

* @return string
*/
private function updateSelectedColumns(array $updateColumns, Columns $columns) : string
{
Copy link
Member

Choose a reason for hiding this comment

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

I think I would use the same approach as above except that I would first filter Columns to only have updateColumns

@norberttech norberttech merged commit 89530c1 into flow-php:1.x Dec 3, 2022
@norberttech
Copy link
Member

Thanks @mleczakm! I left two small comments but they are something that can be improved later, nothing critical, mostly code organization.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants