One very peculiar, but not impossible, case is that the list of mutations only contains empty insert mutations. This could for example be possible for a table that has a default value for the primary key (or allows null for the primary key), and also either has a default value for all other columns, or allows them to be null. Such a case would lead to this method returning Mutation.getDefaultInstance() as the largest insert mutation.

The heuristics should therefore also be changed to prefer an insert mutation with zero columns and a table name, over an insert mutation with zero columns and no table name. (Any other non-empty property of a real mutation can also be used instead of the table name for this check).

Can we also add a test for this specific scenario? It is typically something that could also backslide in the future.

Thanks Knut for pointing out to this edge case.

I have updated the code to handle this case by adding below lines, and added a test for empty insert mutation scenario.

    // If largestInsertMutation is a default instance of Mutation, replace it with the current
    // INSERT mutation, even if it contains zero values.
    if (!largestInsertMutation.hasInsert()) {
      return true;
    }

Talking out loud of all the edge cases,

1. If mutations are empty, then this logic never gets called.
2. Table name in a mutation can not be empty.
3. If there are mutations other than INSERT operation, then once of them is chosen at random.
4. If only INSERT mutations, then one with largest value is chosen
5. If only INSERT mutation, with no columns/values set, then this mutation will be chosen as random mutation

Please let me know if there is anything missing.

You can skip most of this, as the if statement on line 429 already guarantees that this is a DELETE mutation. So you can just directly add it to allMutationsExcludingInsert.

We have a coalescing mechanism to group similar consecutive mutations together.
As a result of this logic, the if statement ensures that the current mutation selected from the list is a DELETE mutation. However, the proto being built is based on prior iterations and will not necessarily be a DELETE mutation.

For example, consider the following mutation list:

Mutation.newInsertBuilder("T1").set("C").to("V1").build(),
Mutation.delete("T1", Key.of("k1")),
Mutation.newInsertBuilder("T2").set("C").to("V1").build()

In this case, the if condition will match the DELETE operation on line 2, but the proto will be an INSERT mutation from line 1.
We have a test case toProtoCoalescingDeleteChanges to verify this behavior.

This code block is repeated a couple of times. Could we extract that to a separate method?

I have extracted them to seperate methods. Please let me know if there are any other better ways.

I think that this should be:

Please add a test that fails with the current implementation and succeeds with the above change (unless I missed something here, and the current implementation is correct).

I just realized that this won't fail, as there are only two possibilities:

1. There are only insert mutations. Then the largest insert mutation will be used.
2. There is at least one non-insert mutation. Although it is possible that that mutation is set as 'the largest insert mutation' with the current if statement, it would not really make any difference, as the largest insert mutation won't be used anyways.

We should still fix the above if condition, though, as the current implementation is a bit confusing.

There is however another (small) optimization possible here; Once allMutationsExcludingInserts.isEmpty() returns false, you can skip the tracking of the largest insert mutation all-together, as it won't be used anyways.

1. Thanks for pointing this out. It does not affect the result, but it is indeed a flaw. Updated the code.
2. Added the optimization.