Clarify merge queries

+++ includes/database/query.inc	2010-07-03 19:38:55 +0000
@@ -1190,6 +868,319 @@ class UpdateQuery extends Query implemen
+ * The following query is built:
+ *
+ * @code ¶
+ * MERGE INTO table_name USING table_name ON (condition)
+ *   WHEN MATCHED THEN
+ *   UPDATE SET column1 = value1 [, column2 = value2 ...]
+ *   WHEN NOT MATCHED THEN
+ *   INSERT (column1 [, column2 ...]) VALUES (value1 [, value2 ...
+ * @endcode

Not quite. The equivalence of that query is built, but that's not the actual SQL that runs. It should be documented as such.

+++ includes/database/query.inc	2010-07-03 19:38:55 +0000
@@ -1190,6 +868,319 @@ class UpdateQuery extends Query implemen
+   * Warning: unlike InsertQuery::fields() this method can called more than
+   * once for different fields. Subsequent additions of the same field throws
+   * an InvalidMergeQueryException.

Grammar error here. I'm not sure what "this method can called more than once" means.

+++ includes/database/query.inc	2010-07-03 19:38:55 +0000
@@ -1190,6 +868,319 @@ class UpdateQuery extends Query implemen
+   * @return InsertQuery
+   *   The called object.
+   */
+  public function insertFields(array $fields, array $values = array()) {

Actually no, it's now returning the MergeQuery, which is the called object, not the InsertQuery. The same error is present elsewhere, too.

+++ includes/database/query.inc	2010-07-03 19:38:55 +0000
@@ -1190,6 +868,319 @@ class UpdateQuery extends Query implemen
+      if (!$this->condition->count()) {
+        throw new InvalidMergeQueryException(t('Invalid merge query: no conditions'));
+      }

We only need count($this->condition) here, as the count() method is automatically called. That's what the Countable interface is for.

+++ includes/database/select.inc	2010-07-03 19:38:55 +0000
@@ -485,6 +485,17 @@ interface SelectQueryInterface extends Q
+  /**
+   * Add FOR UPDATE to the query.
+   *
+   * @param $set
+   *   IF TRUE, FOR UPDATE will be added to the query, if FALSE then it won't.
+   *  ¶
+   * @return QueryConditionInterface
+   *   The called object.
+   */
+  public function forUpdate($set = TRUE);

This should explain what FOR UPDATE means/does, since I suspect most people have never heard of it. I certainly hadn't until the previous thread. :-)

+++ includes/database/select.inc	2010-07-03 19:38:55 +0000
@@ -1014,6 +1035,12 @@ class SelectQuery extends Query implemen
+  public function forUpdate($set = NULL) {

This is different than the interface definition, which defaults to TRUE, not NULL.

+++ includes/database/sqlite/query.inc	2010-07-03 21:23:34 +0000
@@ -119,65 +119,6 @@ class UpdateQuery_sqlite extends UpdateQ
=== added file 'includes/database/sqlite/select.inc'

=== added file 'includes/database/sqlite/select.inc'
--- includes/database/sqlite/select.inc	1970-01-01 00:00:00 +0000

--- includes/database/sqlite/select.inc	1970-01-01 00:00:00 +0000
+++ includes/database/sqlite/select.inc	2010-07-03 19:16:41 +0000

+++ includes/database/sqlite/select.inc	2010-07-03 19:16:41 +0000
+++ includes/database/sqlite/select.inc	2010-07-03 19:16:41 +0000
@@ -0,0 +1,17 @@

The other drivers put their SELECT implementations in query.inc, since they're all very small and we don't need to burn another file I/O hit on them. That said, I'm not sure that it's even necessary to split SelectQuery off anymore since we're still hitting that file on every page request anyway due to the menu system. We'll deal with that later.

Overall, I really like that this patch goes back to the original MERGE definition and works forward from there. It looks like we end up in the same place, though, with the final SQL that runs.

However, architecturally extending Merge off of Update is not the right way to go. Suppose we later on want/need a driver-specific Merge? Does it extend off of MergeQuery or UpdateQuery_foo?

The better approach here is composition. MergeQuery should create an insert query and update query internally and use those as needed, and then extend from Query directly.

Also, this is an API change as seen in the unit tests. I don't quite grok the reasons for that, and since webchick is considering making the next release a beta we really really have to be careful with those. :-(

+++ includes/database/query.inc	2010-08-16 08:53:24 +0000
@@ -1206,6 +880,376 @@ class UpdateQuery extends Query implemen
+   * Adds a set of field->value pairs to be updated.

updateFields() is not documented to take multiple calls the way insertFields() is. Why?

+++ includes/database/query.inc	2010-08-16 08:53:24 +0000
@@ -1206,6 +880,376 @@ class UpdateQuery extends Query implemen
+  /**
+   * Adds a set of field->value pairs to be inserted.
+   *
+   * Warning: unlike InsertQuery::fields() this method can be called more than
+   * once.
+   *

Why can this function be called multiple times, but not others? Why should this one be special?

+++ includes/database/query.inc	2010-08-16 08:53:24 +0000
@@ -1206,6 +880,376 @@ class UpdateQuery extends Query implemen
+   * The fields are copied to the condition, the UPDATE part and the ISNERT
+   * part all. If no other method is called, the UPDATE will become a no-op.

The first comma is a comma-splice. It should probably be a colon.

+++ includes/database/query.inc	2010-08-16 08:53:24 +0000
@@ -1206,6 +880,376 @@ class UpdateQuery extends Query implemen
+      $select = $this->connection->select($this->conditionTable)
+        ->condition($this->condition)
+        ->forUpdate();
+      $select->addExpression('1');

Isn't forUpdate() chainable?

Actually this is all quite reasonable. I think. I didn't get to fully read it. But I think I'm probably OK with the overall thing. I need to look at it again when not dog tired. chx, go ahead and address the above. :-)

Powered by Dreditor.

I just spent quite a while staring at this patch, and I think I finally understand it. :-) Some comments below:

+++ includes/database/query.inc	2010-09-10 21:21:56 +0000
@@ -1206,6 +880,377 @@ class UpdateQuery extends Query implemen
+   * Warning: unlike InsertQuery::fields() this method can be called more than
+   * once, much like UpdateQuery::fields() and MergeQuery::updateFields().

This is not true. UpdateQuery::fields() just does a blanket overwrite.

The actual reason this method can be safely called multiple times is because it can be called from keys() and fields(), and needs to handle that gracefully.

+++ includes/database/query.inc	2010-09-10 21:21:56 +0000
@@ -1206,6 +880,377 @@ class UpdateQuery extends Query implemen
+    // The MergeQuery::fields method sets needsUpdate to TRUE unconditionally,
+    // however if only key() is called, the UPDATE is not necessary, so save
+    // the value of the needsUpdate flag and restore after.
+    $needsUpdate = $this->needsUpdate;
+    $this->fields($fields);
+    $this->needsUpdate = $needsUpdate;

Couldn't this be eliminated by just calling $this->insertFields() and $this->updateFields()? It's only one more line, and removes the need for two lines and an extra function call. :-)

+++ includes/database/query.inc	2010-09-10 21:21:56 +0000
@@ -1206,6 +880,377 @@ class UpdateQuery extends Query implemen
+          $insert = db_insert($this->table)->fields($this->insertFields);

This should be $this->connection->insert(...)

+++ includes/database/query.inc	2010-09-10 21:21:56 +0000
@@ -1206,6 +880,377 @@ class UpdateQuery extends Query implemen
+        $update = db_update($this->table)

This should be $this->connection->update(...)

Other than those, I am OK with this patch. It is an API change but the footprint of it is quite small it and arguably does move the API closer to the actual SQL spec, which we have been trying to use as our baseline.

So let's fix the above, then commit this.

Powered by Dreditor.

Let's get this in before webchick gets antsy and tags something as beta. :-)

Reviewed this again and committed it to CVS HEAD. Thanks.

If you are a human please disregard this issue for now. If you are the testing bot then I want to see what do you say.

BUSTED! We have an issue titled "Clarify merge queries" without even an issue summary.

Please, an issue summary.

This is marked as API change. Does it have BC implications?

"Our previous Merge query implementation was modeled on MySQL's "INSERT ... ON DUPLICATE KEY UPDATE" functionality. However, that did not properly map to the definition of Merge queries in the SQL spec.

The new implementation is modeled directly on the SQL spec's Merge query definition (even if none of the core databases are capable of supporting it directly). In most cases there is no API change. However, code that was using MergeQuery::updateExcept() will need to switch to insertFields() and updateFields(). See the docblock of MergeQuery and the corresponding unit tests for examples."

There's your API change report, Randy. :-) Thanks for following up.

This introduced two (small) regressions, see #943042: MergeQuery regressions: key is used in the update query, exception is echoed.