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

@@ -52,13 +52,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 +74,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 +105,32 @@ 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 (saveStatus.known.deps() == DepsErased)
+                mask |= setFieldIsNullAndChanged(PARTIAL_DEPS, mask);
+            if (saveStatus.known.executeAt() == ExecuteAtErased)
+                mask |= setFieldIsNullAndChanged(EXECUTE_AT, mask);
+            if (saveStatus.known.definition() == DefinitionErased)
+                mask |= setFieldIsNullAndChanged(PARTIAL_TXN, mask);
+            if (saveStatus.known.outcome() == WasApply)
+                mask |= setFieldIsNullAndChanged(RESULT, mask);
+            saveStatusMasks[i] = mask;
+        }
+    }
+
     public static class Builder
     {
         protected final int mask;
@@ -151,72 +181,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 +260,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)
@@ -506,6 +480,10 @@ public static int getFlags(Command before, Command after)
         if (before == null && after == null)
             return flags;
 
+        // TODO (desired): as of now, we don't declare that any of the fields as Erased, even though we have erased them.
+        if (before == null && after.saveStatus() == SaveStatus.Vestigial)
+            before = Command.NotDefined.uninitialised(after.txnId());
+
         // TODO (expected): derive this from precomputed bit masking on Known, only testing equality of objects we can't infer directly
         flags = collectFlags(before, after, Command::executeAt, Timestamp::equalsStrict, true, EXECUTE_AT, flags);
         flags = collectFlags(before, after, Command::executesAtLeast, true, EXECUTES_AT_LEAST, flags);
@@ -531,6 +509,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

@@ -84,6 +84,7 @@
 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;
@@ -172,10 +173,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 +561,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)