Fix null field accounting

Patch by Alex Petrov; reviewed by Benedict Elliott Smith for CASSANDRA-20297

Author: ifesdjeen

accord-core/src/main/java/accord/impl/CommandChange.java

@@ -20,6 +20,7 @@
 
 import java.util.function.BiPredicate;
 import java.util.function.Function;
+import java.util.function.Predicate;
 import java.util.function.ToLongFunction;
 
 import com.google.common.annotations.VisibleForTesting;
@@ -52,13 +53,13 @@
 import static accord.impl.CommandChange.Field.EXECUTES_AT_LEAST;
 import static accord.impl.CommandChange.Field.EXECUTE_AT;
 import static accord.impl.CommandChange.Field.FIELDS;
+import static accord.impl.CommandChange.Field.MIN_UNIQUE_HLC;
 import static accord.impl.CommandChange.Field.PARTIAL_DEPS;
 import static accord.impl.CommandChange.Field.PARTIAL_TXN;
 import static accord.impl.CommandChange.Field.PARTICIPANTS;
 import static accord.impl.CommandChange.Field.PROMISED;
 import static accord.impl.CommandChange.Field.RESULT;
 import static accord.impl.CommandChange.Field.SAVE_STATUS;
-import static accord.impl.CommandChange.Field.MIN_UNIQUE_HLC;
 import static accord.impl.CommandChange.Field.WAITING_ON;
 import static accord.impl.CommandChange.Field.WRITES;
 import static accord.local.Cleanup.NO;
@@ -74,7 +75,11 @@
 import static accord.local.Command.Truncated.invalidated;
 import static accord.local.Command.Truncated.vestigial;
 import static accord.local.StoreParticipants.Filter.LOAD;
+import static accord.primitives.Known.Definition.DefinitionErased;
+import static accord.primitives.Known.KnownDeps.DepsErased;
 import static accord.primitives.Known.KnownExecuteAt.ApplyAtKnown;
+import static accord.primitives.Known.KnownExecuteAt.ExecuteAtErased;
+import static accord.primitives.Known.Outcome.WasApply;
 import static accord.primitives.Status.Durability.NotDurable;
 
 public class CommandChange
@@ -101,6 +106,37 @@ public enum Field
         public static final Field[] FIELDS = values();
     }
 
+    /**
+     * SaveStatus.Known contains information about erased / nullified fields,
+     * which we can use in order to mark the corresponding fields as changed
+     * and setting them to null when they are erased.
+     */
+    public static int[] saveStatusMasks;
+
+    static
+    {
+        saveStatusMasks = new int[SaveStatus.values().length];
+        for (int i = 0; i < saveStatusMasks.length; i++)
+        {
+            SaveStatus saveStatus = SaveStatus.forOrdinal(i);
+            int mask = 0;
+            if (forceFieldChangedToNullFlag(saveStatus, saveStatus.known::is, DepsErased))
+                mask |= setFieldIsNullAndChanged(PARTIAL_DEPS, mask);
+            if (forceFieldChangedToNullFlag(saveStatus, saveStatus.known::is, ExecuteAtErased))
+                mask |= setFieldIsNullAndChanged(EXECUTE_AT, mask);
+            if (forceFieldChangedToNullFlag(saveStatus, saveStatus.known::is, DefinitionErased))
+                mask |= setFieldIsNullAndChanged(PARTIAL_TXN, mask);
+            if (forceFieldChangedToNullFlag(saveStatus, saveStatus.known::is, WasApply))
+                mask |= setFieldIsNullAndChanged(RESULT, mask);
+            saveStatusMasks[i] = mask;
+        }
+    }
+
+    private static <T> boolean forceFieldChangedToNullFlag(SaveStatus saveStatus, Predicate<T> predicate, T erased)
+    {
+        return saveStatus == SaveStatus.Vestigial || predicate.test(erased);
+    }
+
     public static class Builder
     {
         protected final int mask;
@@ -151,72 +187,16 @@ public Builder()
             this(ALL);
         }
 
-        public TxnId txnId()
-        {
-            return txnId;
-        }
-
-        public Timestamp executeAt()
-        {
-            return executeAt;
-        }
-
-        // TODO: why is this unused in BurnTest
-        public Timestamp executeAtLeast()
-        {
-            return executeAtLeast;
-        }
-
         public SaveStatus saveStatus()
         {
             return saveStatus;
         }
 
-        public Status.Durability durability()
-        {
-            return durability;
-        }
-
-        public Ballot acceptedOrCommitted()
-        {
-            return acceptedOrCommitted;
-        }
-
-        public Ballot promised()
-        {
-            return promised;
-        }
-
         public StoreParticipants participants()
         {
             return participants;
         }
 
-        public PartialTxn partialTxn()
-        {
-            return partialTxn;
-        }
-
-        public PartialDeps partialDeps()
-        {
-            return partialDeps;
-        }
-
-        public CommandChange.WaitingOnProvider waitingOn()
-        {
-            return waitingOn;
-        }
-
-        public Writes writes()
-        {
-            return writes;
-        }
-
-        public Result result()
-        {
-            return result;
-        }
-
         public void clear()
         {
             flags = 0;
@@ -286,7 +266,7 @@ public Cleanup shouldCleanup(Input input, Agent agent, RedundantBefore redundant
 
         public Builder maybeCleanup(Cleanup cleanup)
         {
-            if (saveStatus() == null)
+            if (saveStatus == null)
                 return this;
 
             switch (cleanup)
@@ -531,6 +511,10 @@ public static int getFlags(Command before, Command after)
             flags = setChanged(PARTICIPANTS, flags);
             flags = setChanged(SAVE_STATUS, flags);
         }
+
+        if (after.saveStatus() != null)
+            flags |= saveStatusMasks[after.saveStatus().ordinal()];
+
         return flags;
     }
 

accord-core/src/test/java/accord/impl/basic/InMemoryJournal.java

@@ -83,9 +83,8 @@
 import static accord.impl.CommandChange.isNull;
 import static accord.impl.CommandChange.nextSetField;
 import static accord.impl.CommandChange.setChanged;
-import static accord.impl.CommandChange.setFieldIsNull;
+import static accord.impl.CommandChange.setFieldIsNullAndChanged;
 import static accord.impl.CommandChange.toIterableSetFields;
-import static accord.impl.CommandChange.unsetFieldIsNull;
 import static accord.impl.CommandChange.unsetIterable;
 import static accord.impl.CommandChange.validateFlags;
 import static accord.local.Cleanup.Input.FULL;
@@ -172,10 +171,10 @@ private Builder reconstruct(List<Diff> saved, Load load)
         if (saved == null)
             return null;
 
-        // TODO (expected): match C* and visit in reverse order
         Builder builder = null;
-        for (Diff diff : saved)
+        for (int i = saved.size() - 1; i >= 0; i--)
         {
+            Diff diff = saved.get(i);
             if (builder == null)
                 builder = new Builder(diff.txnId, load);
             builder.apply(diff);
@@ -560,70 +559,28 @@ private void apply(Diff diff)
             {
                 Field field = nextSetField(iterable);
 
-                this.flags = setChanged(field, this.flags);
+                // Since we are iterating in reverse order, we skip the fields that were
+                // set by entries writer later (i.e. already read ones).
+                if (isChanged(field, this.flags) || isNull(field, mask))
+                {
+                    iterable = unsetIterable(field, iterable);
+                    continue;
+                }
+
                 if (isNull(field, diff.flags))
                 {
-                    this.flags = setFieldIsNull(field, this.flags);
-                    setNull(field);
+                    this.flags = setFieldIsNullAndChanged(field, this.flags);
                 }
                 else
                 {
-                    this.flags = unsetFieldIsNull(field, this.flags);
+                    this.flags = setChanged(field, this.flags);
                     deserialize(diff, field);
                 }
 
                 iterable = unsetIterable(field, iterable);
             }
         }
 
-        private void setNull(Field field)
-        {
-            switch (field)
-            {
-                case EXECUTE_AT:
-                    executeAt = null;
-                    break;
-                case EXECUTES_AT_LEAST:
-                    executeAtLeast = null;
-                    break;
-                case MIN_UNIQUE_HLC:
-                    minUniqueHlc = 0;
-                    break;
-                case SAVE_STATUS:
-                    saveStatus = null;
-                    break;
-                case DURABILITY:
-                    durability = null;
-                    break;
-                case ACCEPTED:
-                    acceptedOrCommitted = null;
-                    break;
-                case PROMISED:
-                    promised = null;
-                    break;
-                case PARTICIPANTS:
-                    participants = null;
-                    break;
-                case PARTIAL_TXN:
-                    partialTxn = null;
-                    break;
-                case PARTIAL_DEPS:
-                    partialDeps = null;
-                    break;
-                case WAITING_ON:
-                    waitingOn = null;
-                    break;
-                case WRITES:
-                    writes = null;
-                    break;
-                case RESULT:
-                    result = null;
-                    break;
-                case CLEANUP:
-                    throw new IllegalStateException();
-            }
-        }
-
         private void deserialize(Diff diff, Field field)
         {
             switch (field)