insertBatch updateBatch upsertBatch deleteBatch from query

[sclubricants]

This PR adds the final piece of some changes to BaseBuilder.

This adds the ability to insert, update, upsert from a query rather than a set of data.

Much of the required pieces of this PR were already in place.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[sclubricants]

@codeigniter4/database-team This is the last major piece for now. Please review.

[sclubricants]

Perhaps it might be better to not pass a query through the $set parameter of th *Batch() methods and rely on using the fromQuery() method only. The $set parameter is really the same as in setData().

[sclubricants]

It should be noted that this design works on the assumption that the columns in the query are aliased to match those of the target table. While SQL usually works based on position and doesn't require the column names to match it is usually good practice as queries are much more readable. It also makes it much simpler to handle an upsert scenario where the target and source share the same name.. otherwise the developer would have to map them separately.
This also makes it possible to know which columns are used for the query.

[sclubricants]

Example:

This is valid SQL:

INSERT INTO pageview (user_id, user_name, last_view)
SELECT id, name, created_date
FROM user
ON DUPLICATE KEY UPDATE
SET user_id = VALUES(id),
user_name = VALUES(name),
last_view = VALUES(created_date)

The problem with this is that the fields would have to be mapped: user_id => id

This PR will assume this is done like:

INSERT INTO pageview (user_id, user_name, last_view)
SELECT id AS user_id, name AS user_name, created_date AS last_view
FROM user
ON DUPLICATE KEY UPDATE
SET user_id = VALUES(user_id ),
user_name = VALUES(user_name ),
last_view = VALUES(last_view )

This way the mapping is done in SQL and allows to easily generate the SQL statements. The code will parse the field names from the select statement and use them to generate the other parts of the statement.

[kenjis]

Sorry, we reorganized the changelog v4.3.0.
Please rebase.

[kenjis]

Do we really need this public method?

[sclubricants]

I prefer to call the method directly rather than pass it in the *Batch function. I think it is easier to read the code when you do it this way.

$this->db->table('user')->insertBatch($query);

$this->db->table('user')->fromQuery($query)->insertBatch();

$this->db->table('user')->setData($data)->insertBatch();

I also debated weather or not I should accept queries in the $set parameter of the *Batch() methods. $set was really intended as the parameter for setData(). But it works passing query in *Batch() methods.

At some point It might be useful for other methods as well. We could always change it to public later I suppose though.

[michalsn]

This may be a dumb question, but do we need this new method? I mean.. we have fromSubquery() - shouldn't this be handled by this method somehow? It doesn't look good from the API perspective.

[sclubricants]

fromSubquery() works a bit differently. It sets the query to QBFrom[0] . fromQuery() sets the query to QBOptions['fromQuery'] and QBKeys to the columns.

One option might be to move the fromQuery() logic to setData(). This kind of makes sense programming wise.. the query is an alternate to a dataset. The only problem might be is that people might try and use it where it is not supported.

[kenjis]

The following API seems better if it is possible, because we have fromSubquery() already:

$this->db->table('user')->fromSubquery($query)->insertBatch();

fromSubquery() works a bit differently. It sets the query to QBFrom[0] . fromQuery() sets the query to QBOptions['fromQuery'] and QBKeys to the columns.

It is just internal implementation. We can change the implementation.

[sclubricants]

setDataAsQuery()

This is what setData() actually does.

fromQuery() was ok but it might get confused with fromSubQuery()

In the future I'd like to have a method that is something like queryAsTable() This would be similar to what fromSubQuery() does in that it puts the query in parenthesis and adds an alias. Basically the results of this function could go anywhere a table name could go. This method could be used instead of fromSubQuery() by just setting table(). It could also go in join().

setQueryAsData() is the only other option I see that might make sense.

One thing to keep in mind is that the API language doesn't have to necessarily follow the SQL. I mean for the most part yes it is nice but with some of these more advanced SQL statements they vary with each DBMS. I liked fromQuery() because it seemed clear that the data was coming from the query.

[michalsn]

Both fromQuery() and setQueryAsData() seem right to me because they describe well what will happen.

The only disadvantage of fromQuery() is that it has a very similar name to an already existing method, and people may be confused about why they're working differently.

For that reason, I would go with setQueryAsData().

[sclubricants]

For that reason, I would go with setQueryAsData().

I agree. I think this clearly presents itself as an alternative to setData().

I changed everything to this.

@kenjis how do you feel about this?

[kenjis]

setQueryAsData() is much better than setData() and setQuery().

[sclubricants]

Normally with *Batch you use setData(). setQueryAsData() is a fitting alternative. It is self explanatory.

[kenjis]

@codeigniter4/database-team
My question is:
Do we really need to accept a query with *batch() methods?
See #6689 (review)

My opinion is:
If a dev want to set a query as data, they should use the setQueryAsData() method.

[kenjis]

If I specify two queries as follows, how should QueryBuilder behave?

$builder->setQueryAsData($query1)->onConstraint('email')
->updateFields($additionalUpdateField, true)->upsertBatch($query2)

[michalsn]

If I specify two queries as follows, how should QueryBuilder behave?

$builder->setQueryAsData($query1)->onConstraint('email')
->updateFields($additionalUpdateField, true)->upsertBatch($query2)

I think this is pretty straightforward - this would be the equivalent of:

$builder->setQueryAsData($query1)->setQueryAsData($query2)->onConstraint('email')
->updateFields($additionalUpdateField, true)->upsertBatch();

So only $query2 would be used.

The same story is with onConstraint(). It also can be set via upsertBatch() or deleteBatch().

But... from the other side, *Batch() methods would be the only ones that allow this type of data, leading us to inconsistency. From that point of view, I think I agree with @kenjis. BaseBuilder and RawSql as a parameter should be treated as special cases and allowed to be set via a dedicated method only.

[kenjis]

It is not bad idea that set*() method will override the existing data, because it is set not add.
But in CI set*() method may add the data. See #6536, #5674
Well, it is not consistent.

[michalsn]

Yes, with, let's say array data, set*() will add or override existing keys. And it has sense.

But with a setQueryAsData() or onConstraint(), we can only have one instance, so the value will always be replaced. This might be worth mentioning in the docs.

[sclubricants]

I think that the singularity of the method's name suggests that it can only be used once. Its not setQueriesAsData().

I have removed the BaseBuilder and RawSql inputs from $set parameter in *Batch() methods. I agree that it is a special use case and should not be included. The parameter types should mirror that of setData() and not setQueryAsData().

[michalsn]

LGTM

[kenjis]

LGTM!

[MGatner]

🥳🎉🎊🕺