chore(spanner): review comments

Author: harshachinta

google-cloud-spanner/src/main/java/com/google/cloud/spanner/Mutation.java

@@ -408,11 +408,12 @@ private boolean isFloat32NaN(Value value) {
    * Converts the list of mutations to the corresponding protobuf mutations and returns a random
    * mutation from the available list based on the following heuristics:
    *
-   * <p>1. Prefer mutations other than INSERT, as INSERT mutations may contain autogenerated columns
-   * whose information is unavailable on the client.
-   *
-   * <p>2. If the list only contains INSERT mutations, select the one with the highest number of
-   * values.
+   * <ol>
+   *   <li>1. Prefer mutations other than INSERT, as INSERT mutations may contain autogenerated
+   *       columns whose information is unavailable on the client.
+   *   <li>If the list only contains INSERT mutations, select the one with the highest number of
+   *       values.
+   * </ol>
    */
   static com.google.spanner.v1.Mutation toProtoAndReturnRandomMutation(
       Iterable<Mutation> mutations, List<com.google.spanner.v1.Mutation> out) {
@@ -437,10 +438,12 @@ static com.google.spanner.v1.Mutation toProtoAndReturnRandomMutation(
           if (proto != null) {
             com.google.spanner.v1.Mutation builtMutation = proto.build();
             out.add(builtMutation);
-            if (checkIfInsertMutationWithLargeValue(builtMutation, largestInsertMutation)) {
+            // Skip tracking the largest insert mutation if there are mutations other than INSERT.
+            if (allMutationsExcludingInsert.isEmpty()
+                && checkIfInsertMutationWithLargeValue(builtMutation, largestInsertMutation)) {
               largestInsertMutation = builtMutation;
             }
-            addMutationsExcludingInsert(builtMutation, allMutationsExcludingInsert);
+            maybeAddMutationToListExcludingInserts(builtMutation, allMutationsExcludingInsert);
           }
           proto = com.google.spanner.v1.Mutation.newBuilder();
           com.google.spanner.v1.Mutation.Delete.Builder delete =
@@ -464,10 +467,12 @@ static com.google.spanner.v1.Mutation toProtoAndReturnRandomMutation(
           if (proto != null) {
             com.google.spanner.v1.Mutation builtMutation = proto.build();
             out.add(builtMutation);
-            if (checkIfInsertMutationWithLargeValue(builtMutation, largestInsertMutation)) {
+            // Skip tracking the largest insert mutation if there are mutations other than INSERT.
+            if (allMutationsExcludingInsert.isEmpty()
+                && checkIfInsertMutationWithLargeValue(builtMutation, largestInsertMutation)) {
               largestInsertMutation = builtMutation;
             }
-            addMutationsExcludingInsert(builtMutation, allMutationsExcludingInsert);
+            maybeAddMutationToListExcludingInserts(builtMutation, allMutationsExcludingInsert);
           }
           proto = com.google.spanner.v1.Mutation.newBuilder();
           switch (mutation.operation) {
@@ -496,10 +501,12 @@ static com.google.spanner.v1.Mutation toProtoAndReturnRandomMutation(
     if (proto != null) {
       com.google.spanner.v1.Mutation builtMutation = proto.build();
       out.add(proto.build());
-      if (checkIfInsertMutationWithLargeValue(builtMutation, largestInsertMutation)) {
+      // Skip tracking the largest insert mutation if there are mutations other than INSERT.
+      if (allMutationsExcludingInsert.isEmpty()
+          && checkIfInsertMutationWithLargeValue(builtMutation, largestInsertMutation)) {
         largestInsertMutation = builtMutation;
       }
-      addMutationsExcludingInsert(builtMutation, allMutationsExcludingInsert);
+      maybeAddMutationToListExcludingInserts(builtMutation, allMutationsExcludingInsert);
     }
 
     // Select a random mutation based on the heuristic.
@@ -518,7 +525,7 @@ private static boolean checkIfInsertMutationWithLargeValue(
       com.google.spanner.v1.Mutation largestInsertMutation) {
     // If largestInsertMutation is a default instance of Mutation, replace it with the current
     // INSERT mutation, even if it contains zero values.
-    if (!largestInsertMutation.hasInsert()) {
+    if (mutation.hasInsert() && !largestInsertMutation.hasInsert()) {
       return true;
     }
     return mutation.hasInsert()
@@ -527,7 +534,7 @@ private static boolean checkIfInsertMutationWithLargeValue(
   }
 
   // Stores all mutations that are not of type INSERT.
-  private static void addMutationsExcludingInsert(
+  private static void maybeAddMutationToListExcludingInserts(
       com.google.spanner.v1.Mutation mutation,
       List<com.google.spanner.v1.Mutation> allMutationsExcludingInsert) {
     if (!mutation.hasInsert()) {

google-cloud-spanner/src/test/java/com/google/cloud/spanner/MutationTest.java

@@ -530,7 +530,6 @@ public void toProtoWithEmptyInsertMutations() {
     List<com.google.spanner.v1.Mutation> proto = new ArrayList<>();
     com.google.spanner.v1.Mutation mutation =
         Mutation.toProtoAndReturnRandomMutation(mutations, proto);
-    System.out.println(mutation);
 
     // Random mutation returned should be of INSERT operation with empty values
     MatcherAssert.assertThat(mutation, matchesProto("insert { table: 'T' values { } }"));