Need to be able to lookup destination IDs by partial source IDs

To expand on the hashing issue a bit... Right now, the hash stored in the map is a hash of all source keys, and lookupDestinationId() hashes its argument and queries against the table's hash value. There are a couple of ways the new partial-lookup API could work:

1. Ignore the hash and build a query with conditions on only the specific source keys provided. The disadvantage is that this API would not work for the use cases that required the hashing in the first place (such as odd chars in a key value)
2. Save multiple hashes in the map table - the current full-key hash, a hash of just the first key column, a hash of the first two (if there were three), ... Disadvantages would be the added overhead to the map table (for multi-column source keys), and your partial lookup could only be on leading columns (e.g., you wouldn't be able to query on the third source ID column alone) unless we generated hashes for all combinations (not very scalable).

I'm not entirely satisfied with either case - any other ideas?

I do have to say, between those two implementation approaches, I favor #1 - it's simpler, has less overhead, and the downside scenario is a combination of multiple edge cases - it would only affect those who have a source key column which is problematic, which is one of multiple columns, and they need to do a partial query that includes the problematic column.

If hashing is required for some use cases, option 2 seems to be the way. But I would store the hash of each key columns. There is no need to store every possible combos (that would grow very fast) So for instance, for an entry whose IDs is ['tid' => 2, 'language' => 'en'] I would store three hashes (['tid' => 2, 'language' => 'en'], 2 and english). To avoid cluttering the map table, a secondary table could be used for the additional hashes. The main map table would stay the same, and the hashes table would have a column for each key, plus a column for the full keys hash (used a foreign key to the main map table). That table would only be used when required for a lookupDestinationIdsSubset() query.

That saids, I'm curious of the use cases for the hashes, dealing with data and querying them is the job of the DB, having a complex hashes system on top of the DB looks like a bad pattern to me.

@pbuyle: For the history of the hashing, see #2613878: Use hash for Migration source keys, rather than verbatim values.

Ok, so it looks like the hash thing is primary because the keys were used a PK on the table. So it is probably safe to use the non-hashed keys in condition. And there is no need to hash each key individually.

Yay option #1! Thanks for pointing that out @pbuyle...

A note - at first glance, it would seem that we could just make lookupDestinationId() more flexible, to recognize when the source IDs passed in are incomplete and do the alternate on-these-values-only query. The problem there is that lookupDestinationId() currently returns one destination ID (an array containing the ID values for one destination object), while a partial lookup could potentially return multiple destination IDs. In the use case that's driving this, we just need one, but logically the return should be an array of destination ID arrays. So, we proposed a new API for the sake of BC. Of course, we are still experimental, so we can break BC - we don't want to make gratuitous API breaks, but having a special lookupDestinationIdsSubset() doing almost exactly the same thing as lookupDestinationId() would be enough of an API WTF that I'm thinking now we should go ahead and make the change...

And saying that, actually we should add a lookupDestinationIds() (note the plural) which returns an array of destination ID arrays, change lookupDestinationId() to call lookupDestinationIds() and return the first destination ID, and deprecate lookupDestinationId().

It doesn't include the obvious optimization that when all fields are specified, we could use the primary key (source_ids_hash) for lookup.

Let's do that.

But he suggested that if we do decide to optimize, we could allow source plugins to add an 'indexable' property to their fields as returned by getIds(), which the ID map could then respect.

Let's do that (in a follow-up issue).

1. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
   @@ -209,11 +209,25 @@ public function lookupSourceID(array $destination_id_values);
   +   *   The destination identifier values of the record, or empty on failure.
       */
   

   Let's add an @deprecated here.

2. +++ b/core/modules/migrate/src/Plugin/MigrateIdMapInterface.php
   @@ -209,11 +209,25 @@ public function lookupSourceID(array $destination_id_values);
   +   * Looks up the destination identifies corresponding to a source key.
   

   identifiers.

3. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -517,16 +517,46 @@ public function lookupSourceID(array $destination_id_values) {
   +    $is_associative = !isset($source_id_values[0]);
   

   Accepting unkeyed values is an (already-existing) undocumented feature - shall we document it in the interface?

4. +++ b/core/modules/migrate/src/Plugin/migrate/id_map/Sql.php
   @@ -517,16 +517,46 @@ public function lookupSourceID(array $destination_id_values) {
   +    if (!empty($source_id_values)) {
   +      throw new MigrateException("Extra unknown items in source IDs " . print_r($orig_source_ids, TRUE));
   +    }
   

   Let's add an @throws for this.

5. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
   @@ -431,6 +431,111 @@ public function testLookupDestinationIdMapping($num_source_fields, $num_destinat
   +    }
   

   Should assert the exception message.

6. +++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapTest.php
   @@ -431,6 +431,111 @@ public function testLookupDestinationIdMapping($num_source_fields, $num_destinat
   +    }
   

   Should assert the exception message.

Thanks vasi!

Looks good - I think we should have a change record indicated the deprecation and showing how to substitute the new API, with that it's RTBC from my standpoint.

Thanks @vasi!

Draft change record added.

Nice test and nice solution. Committed b073cee and pushed to 8.1.x and 8.2.x. Thanks!

I agree that the question about indexes can be handled in a followup.

Commenting late, but per our conversation in IRC from last week, this should be fine. The only reason for storing the hash instead of the actual values is because of Indexing. Indexes essentially don't work when non standard language sets. Migrations are slow already, so making it slower isn't all that terrible. Someone can always micro-tune the DB if they need to.

I've now published the change record.