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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[QueryBuilder] The combination of the setUpdateBatch() and the updateBatch() is difficult to use. #5134

Closed
ytetsuro opened this issue Sep 26, 2021 · 7 comments
Labels
database Issues or pull requests that affect the database layer

Comments

@ytetsuro
Copy link
Contributor

ytetsuro commented Sep 26, 2021

The combination of the setUpdateBatch method and the updateBatch method is difficult to use.

If you try to use it, it will look like the following.

$builder->setUpdateBatch([
    [
        'id'          => 2,
        'name'        => 'SUBSTRING(name, 1)',
        'description' => 'SUBSTRING(description, 3)',
    ],
    [
        'id'          => 3,
        'name'        => 'SUBSTRING(name, 2)',
        'description' => 'SUBSTRING(description, 4)',
    ],
], 'id', false);

$builder->updateBatch(null, 'id', 100);
  1. updateBatch method hasn't escape argument.
  2. We have specified index as the second argument of the setUpdateBatch method, but we need to specify the same value for the second argument of updateBatch.
  3. In combination with 2, when using setUpdateBatch, you need to specify null as the first argument of the updateBatch method. It is difficult for a client to write such a code without knowing the implementation.

In my opinion, it would be easier for developers to use if they can write the following.

$builder->updateBatch([
    [
        'id'          => 2,
        'name'        => 'SUBSTRING(name, 1)',
        'description' => 'SUBSTRING(description, 3)',
    ],
    [
        'id'          => 3,
        'name'        => 'SUBSTRING(name, 2)',
        'description' => 'SUBSTRING(description, 4)',
    ],
], 'id', false, 100);

or

 $builder->setUpdateBatch([
    [
        'id'          => 2,
        'name'        => 'SUBSTRING(name, 1)',
        'description' => 'SUBSTRING(description, 3)',
    ],
    [
        'id'          => 3,
        'name'        => 'SUBSTRING(name, 2)',
        'description' => 'SUBSTRING(description, 4)',
    ],
], 'id', false);

$builder->updateBatch(100);

What do you think?

@kenjis
Copy link
Member

kenjis commented Sep 28, 2021

In both cases, it is a breaking change in the public API of updateBatch().

@ytetsuro
Copy link
Contributor Author

ytetsuro commented Sep 28, 2021

Thanks, response.
You are right.

@ytetsuro
Copy link
Contributor Author

ytetsuro commented Oct 11, 2021

@kenjis

I was wondering if the following code could be implemented without adding breaking change if the user has called setUpdateBatch at least once.

The concrete method is to change the behavior of updateBatch when the following conditions are met.

  • Only the first argument of updateBatch is specified.
  • A number is passed as the first argument of updateBatch.
  • QBSet is not empty.
 $builder->setUpdateBatch([
    [
        'id'          => 2,
        'name'        => 'SUBSTRING(name, 1)',
        'description' => 'SUBSTRING(description, 3)',
    ],
    [
        'id'          => 3,
        'name'        => 'SUBSTRING(name, 2)',
        'description' => 'SUBSTRING(description, 4)',
    ],
], 'id', false);

$builder->updateBatch(100);

@ytetsuro ytetsuro reopened this Oct 11, 2021
@kenjis
Copy link
Member

kenjis commented Oct 11, 2021

@ytetsuro I think the API you propose is better. It is easy to use.

But unfortunately, the first param of updateBatch() is ?array.

public function updateBatch(?array $set = null, ?string $index = null, int $batchSize = 100)

If you change it to accept int, it is breaking change.
https://3v4l.org/oIIBP

@kenjis
Copy link
Member

kenjis commented Oct 11, 2021

You could write code like this on PHP8:

$builder->updateBatch(batchSize: 100);

https://www.php.net/manual/en/functions.arguments.php#functions.named-arguments

@kenjis
Copy link
Member

kenjis commented Oct 12, 2021

Or how about this?

$builder->setUpdateBatch([
    [
        'id'          => 2,
        'name'        => 'SUBSTRING(name, 1)',
        'description' => 'SUBSTRING(description, 3)',
    ],
    [
        'id'          => 3,
        'name'        => 'SUBSTRING(name, 2)',
        'description' => 'SUBSTRING(description, 4)',
    ],
], 'id', false);

$builder->setBatchSize(100);
$builder->updateBatch();

@ytetsuro
Copy link
Contributor Author

@kenjis

Thanks, response.

If you change it to accept int, it is breaking change.

Sure!
I'm sorry.😞

You could write code like this on PHP8:

Oh! You are great!
This is very readable.

Or how about this?

Thanks.
I think very very well.😍
I will make this PR.

@kenjis kenjis changed the title The combination of the setUpdateBatch method and the updateBatch method is difficult to use. [QueryBuilder] The combination of the setUpdateBatch() and the updateBatch() is difficult to use. Jul 29, 2022
@kenjis kenjis added the database Issues or pull requests that affect the database layer label Jul 29, 2022
@ytetsuro ytetsuro closed this as completed Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer
Projects
None yet
Development

No branches or pull requests

2 participants