Repository: amzn/ion-java
Pull request #221 (2019-03-15 21:28:29)

IonValueLite.clearSymbolIdValues() performance fix
#220


codecov[bot]: # [Codecov](https://codecov.io/gh/amzn/ion-java/pull/221?src=pr&el=h1) Report
> Merging [#221](https://codecov.io/gh/amzn/ion-java/pull/221?src=pr&el=desc) into [master](https://codecov.io/gh/amzn/ion-java/commit/36c370bf658dc697ad8fbdd6fbfe7eaac911a38d?src=pr&el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `92.45%`.

[![Impacted file tree graph](https://codecov.io/gh/amzn/ion-java/pull/221/graphs/tree.svg?width=650&token=M93Y3e3kKn&height=150&src=pr)](https://codecov.io/gh/amzn/ion-java/pull/221?src=pr&el=tree)

```diff
@@             Coverage Diff              @@
##             master     #221      +/-   ##
============================================
+ Coverage     72.28%   72.33%   +0.04%     
- Complexity     4349     4369      +20     
============================================
  Files           121      121              
  Lines         16028    16070      +42     
  Branches       2638     2652      +14     
============================================
+ Hits          11586    11624      +38     
  Misses         3310     3310              
- Partials       1132     1136       +4
```


| [Impacted Files](https://codecov.io/gh/amzn/ion-java/pull/221?src=pr&el=tree) | Coverage Δ | Complexity Δ | |
|---|---|---|---|
| [...oftware/amazon/ion/impl/lite/IonContainerLite.java](https://codecov.io/gh/amzn/ion-java/pull/221/diff?src=pr&el=tree#diff-c3JjL3NvZnR3YXJlL2FtYXpvbi9pb24vaW1wbC9saXRlL0lvbkNvbnRhaW5lckxpdGUuamF2YQ==) | `78.03% <100%> (+0.42%)` | `70 <2> (+2)` | :arrow_up: |
| [...c/software/amazon/ion/impl/lite/IonSymbolLite.java](https://codecov.io/gh/amzn/ion-java/pull/221/diff?src=pr&el=tree#diff-c3JjL3NvZnR3YXJlL2FtYXpvbi9pb24vaW1wbC9saXRlL0lvblN5bWJvbExpdGUuamF2YQ==) | `78.84% <85.71%> (-0.33%)` | `38 <5> (+2)` | |
| [...rc/software/amazon/ion/impl/lite/IonValueLite.java](https://codecov.io/gh/amzn/ion-java/pull/221/diff?src=pr&el=tree#diff-c3JjL3NvZnR3YXJlL2FtYXpvbi9pb24vaW1wbC9saXRlL0lvblZhbHVlTGl0ZS5qYXZh) | `81.14% <93.33%> (+1.02%)` | `139 <17> (+16)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/amzn/ion-java/pull/221?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/amzn/ion-java/pull/221?src=pr&el=footer). Last update [36c370b...bf5bda1](https://codecov.io/gh/amzn/ion-java/pull/221?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/src/software/amazon/ion/impl/lite/IonContainerLite.java b/src/software/amazon/ion/impl/lite/IonContainerLite.java
index a77b5834..d8fe748d 100644
--- a/src/software/amazon/ion/impl/lite/IonContainerLite.java
+++ b/src/software/amazon/ion/impl/lite/IonContainerLite.java
@@ -49,6 +49,7 @@ abstract class IonContainerLite
 
     IonContainerLite(IonContainerLite existing, IonContext context, boolean isStruct) {
         super(existing, context);
+        boolean retainingSIDs = false;
         int childCount = existing._child_count;
         this._child_count = childCount;
         // when cloning the children we establish 'this' the cloned outer container as the context
@@ -76,7 +77,13 @@ abstract class IonContainerLite
                     }
                 }
                 this._children[i] = copy;
+                retainingSIDs |= copy._isSymbolIdPresent();
             }
+            // unfortunately due to the existing behavior in IonValueLite copy-constructor where annotation SID's are
+            // preserved across the copy-constructor IF they have no resolved text it means that encodings could have
+            // been preserved on the child - therefore the cloned children each have to be re-interrogated and the
+            // setting updated IF such a change has occurred.
+            _isSymbolIdPresent(retainingSIDs);
         }
     }
 
@@ -432,13 +439,19 @@ abstract class IonContainerLite
     }
 
     @Override
-    void clearSymbolIDValues()
+    boolean attemptClearSymbolIDValues()
     {
-        super.clearSymbolIDValues();
-        for (int ii=0; ii<get_child_count(); ii++) {
+        boolean symbolIDsAllCleared = super.attemptClearSymbolIDValues();
+
+        for (int ii = 0; ii < get_child_count(); ii++)
+        {
             IonValueLite child = get_child(ii);
-            child.clearSymbolIDValues();
+            // NOTE: recursion is done to #clearSymbolIDValues rather than #attemptClearSymbolIDValues in order to
+            // set the SYMBOL ID PRESENT status flag correctly.
+            symbolIDsAllCleared &= child.clearSymbolIDValues();
         }
+
+        return symbolIDsAllCleared;
     }
 
     /**
@@ -660,6 +673,12 @@ abstract class IonContainerLite
         structuralModificationCount++;
 
         child._elementid(idx);
+
+        if (!_isSymbolIdPresent() && child._isSymbolIdPresent())
+        {
+            cascadeSIDPresentToContextRoot();
+        }
+
         return idx;
     }
 
diff --git a/src/software/amazon/ion/impl/lite/IonSymbolLite.java b/src/software/amazon/ion/impl/lite/IonSymbolLite.java
index 3f73a1ca..8f13f8eb 100644
--- a/src/software/amazon/ion/impl/lite/IonSymbolLite.java
+++ b/src/software/amazon/ion/impl/lite/IonSymbolLite.java
@@ -14,15 +14,15 @@
 
 package software.amazon.ion.impl.lite;
 
+import software.amazon.ion.IonType;
+import software.amazon.ion.IonWriter;
+import software.amazon.ion.SymbolTable;
 import static software.amazon.ion.SymbolTable.UNKNOWN_SYMBOL_ID;
+import software.amazon.ion.SymbolToken;
 import static software.amazon.ion.SystemSymbols.ION_1_0;
 import static software.amazon.ion.SystemSymbols.ION_1_0_SID;
 
 import java.io.IOException;
-import software.amazon.ion.IonType;
-import software.amazon.ion.IonWriter;
-import software.amazon.ion.SymbolTable;
-import software.amazon.ion.SymbolToken;
 import software.amazon.ion.UnknownSymbolException;
 import software.amazon.ion.ValueVisitor;
 import software.amazon.ion.impl.PrivateIonSymbol;
@@ -62,16 +62,14 @@ final class IonSymbolLite
             if (text != null)
             {
                 super.setValue(text);
-
-                // TODO why is the sid ignored in this case?
-                // Probably because we don't trust our ability to change the
-                // sid if this value gets recontextualized.
-                // BUT: we retain the sid on field names and annotations,
-                // so one or the other is buggy.
+                // TODO [ION-320] - needs consistent handling, when to retain SID's vs ignore
             }
             else
             {
+                // TODO [IONJAVA-579] - needs consistent handling, resolution against context symbol table
                 _sid = sid;
+                // there *is* an encoding present so we must update
+                _isSymbolIdPresent(true);
             }
         }
     }
@@ -130,6 +128,7 @@ final class IonSymbolLite
         return IonType.SYMBOL;
     }
 
+
     /**
      * Get's the text of this NON-NULL symbol, finding it from our symbol
      * table if it's not yet known (and caching the result if possible).
@@ -173,22 +172,26 @@ final class IonSymbolLite
 
     private int resolveSymbolId(SymbolTableProvider symbolTableProvider)
     {
+
         validateThisNotNull();
 
         if (_sid != UNKNOWN_SYMBOL_ID || isReadOnly()) {
             return _sid;
         }
 
-        SymbolTable symtab = symbolTableProvider.getSymbolTable();
+        SymbolTable symtab =
+            symbolTableProvider != null ? symbolTableProvider.getSymbolTable()
+                : getSymbolTable();
         if (symtab == null) {
             symtab = getSystem().getSystemSymbolTable();
         }
         assert(symtab != null);
 
         String name = _get_value();
+        // TODO [ION-320] - needs consistent handling, when to retain SID's vs ignore (here memoizing SID on read)
         if (!symtab.isLocalTable())
         {
-            _sid = symtab.findSymbol(name);
+            setSID(symtab.findSymbol(name));
             if (_sid > 0 || isReadOnly()) {
                 return _sid;
             }
@@ -196,12 +199,21 @@ final class IonSymbolLite
         SymbolToken tok = symtab.find(name);
         if (tok != null)
         {
-            _sid = tok.getSid();
+            setSID(tok.getSid());
             _set_value(tok.getText()); // Use the interned instance of the text
         }
         return _sid;
     }
 
+    private void setSID(int sid)
+    {
+        _sid = sid;
+        if (_sid > 0)
+        {
+            cascadeSIDPresentToContextRoot();
+        }
+    }
+
     public SymbolToken symbolValue(SymbolTableProvider symbolTableProvider)
     {
         if (isNullValue()) return null;
@@ -224,15 +236,28 @@ final class IonSymbolLite
     }
 
     @Override
-    void clearSymbolIDValues()
+    boolean attemptClearSymbolIDValues()
     {
-        super.clearSymbolIDValues();
+        boolean allSymbolIDsCleared = super.attemptClearSymbolIDValues();
 
-        // Don't lose the sid if that's all we have!
-        if (! isNullValue() && _stringValue() != null)
+        // if there is no value, or there is already no SID - there is no clear action
+        if (isNullValue() || _sid == UNKNOWN_SYMBOL_ID)
+        {
+            // no behavior required - value has no SID value to clear
+        }
+        // if there is text, clear the SID
+        else if (_stringValue() != null)
         {
             _sid = UNKNOWN_SYMBOL_ID;
         }
+        else
+        {
+            // TODO [IONJAVA-579] - needs consistent handling, resolution against context symbol table
+            // there is not text, so we can't clear the SID.
+            allSymbolIDsCleared = false;
+        }
+
+        return allSymbolIDsCleared;
     }
 
     protected void setIsIonVersionMarker(boolean isIVM)
@@ -246,7 +271,6 @@ final class IonSymbolLite
         _sid = ION_1_0_SID;
     }
 
-
     @Override
     final void writeBodyTo(IonWriter writer, SymbolTableProvider symbolTableProvider)
         throws IOException
diff --git a/src/software/amazon/ion/impl/lite/IonValueLite.java b/src/software/amazon/ion/impl/lite/IonValueLite.java
index cc2e80de..c0245cbc 100644
--- a/src/software/amazon/ion/impl/lite/IonValueLite.java
+++ b/src/software/amazon/ion/impl/lite/IonValueLite.java
@@ -71,6 +71,16 @@ abstract class IonValueLite
     protected static final int IS_IVM             = 0x10;
     protected static final int IS_AUTO_CREATED    = 0x20;
     protected static final int IS_SYMBOL_PRESENT  = 0x40;
+    /**
+     * Symbol ID present refers to there being the <i>possibility</i> that the IonValueLite, or it's contained sub graph
+     * <i>(e.g. if it is a {@link IonContainerLite} based sub type)</i> contains one or more defined
+     * Symbol ID's (SID)'s. This flag is used to track lifecycle, such that operations that require SID's are purged
+     * from the IonValueLite and it's contained sub-DOM can conduct a fast evaluation rather than having to do a full
+     * contained graph traversal on each and every invocation. For more detail on impact see
+     * <a href="https://issues.amazon.com/issues/IONPT-8">IONPT-8</a>
+     */
+    protected static final int IS_SYMBOL_ID_PRESENT = 0x80;
+
     private   static final int ELEMENT_MASK       = 0xff;
     protected static final int ELEMENT_SHIFT      = 8; // low 8 bits is flag, upper 24 (or 48 is element id)
 
@@ -200,6 +210,17 @@ abstract class IonValueLite
         return flag;
     }
 
+    protected final boolean _isSymbolIdPresent() { return is_true(IS_SYMBOL_ID_PRESENT); }
+    protected final boolean _isSymbolIdPresent(boolean flag) {
+        if (flag) {
+            set_flag(IS_SYMBOL_ID_PRESENT);
+        }
+        else {
+            clear_flag(IS_SYMBOL_ID_PRESENT);
+        }
+        return flag;
+    }
+
     /**
      * Lazy memoized symtab provider. Should be used when a call path
      * conditionally needs access to a value's symbol table. This provider
@@ -283,6 +304,7 @@ abstract class IonValueLite
      */
     IonValueLite(IonValueLite existing, IonContext context) {
         // Symbols are *immutable* therefore a shallow copy is sufficient
+        boolean hasSIDsRetained = false;
         if (null == existing._annotations) {
             this._annotations = null;
         } else {
@@ -296,9 +318,9 @@ abstract class IonValueLite
                         this._annotations[i] =
                             PrivateUtils.newSymbolToken(text, UNKNOWN_SYMBOL_ID);
                     } else {
-                        // TODO - this is clearly wrong; however was the existing behavior as
-                        // existing under #getAnnotationTypeSymbols();
+                        // TODO - IONJAVA-579 needs consistent handling, should attempt to resolve and if it cant; fail
                         this._annotations[i] = existing._annotations[i];
+                        hasSIDsRetained |= this._annotations[i].getSid() > UNKNOWN_SYMBOL_ID;
                     }
                 }
             }
@@ -310,6 +332,10 @@ abstract class IonValueLite
         // as IonValue.clone() mandates that the returned value is mutable, regardless of the
         // existing 'read only' flag - we force the deep-copy back to being mutable
         clear_flag(IS_LOCKED);
+        // whilst the clone *should* guarantee symbol context is purged, the annotation behavior existing above
+        // under the TO DO for IONJAVA-579 does mean that SID context can be propogated through a clone, therefore
+        // the encoding flag has to reflect this reality
+        _isSymbolIdPresent(hasSIDsRetained);
     }
 
     public abstract void accept(ValueVisitor visitor) throws Exception;
@@ -359,6 +385,10 @@ abstract class IonValueLite
                 _annotations[ii] = null;
             }
         }
+
+        // although annotations have been removed from the node, which *may* mean the sub-graph from this Node is
+        // now without encodings... the check is expensive for container types (need to check all immediate children)
+        // and so we will opt to clear down encoding present in a lazy fashion (e.g. when something actually needs it)
     }
 
     /**
@@ -475,15 +505,41 @@ abstract class IonValueLite
         return token;
     }
 
+    final boolean clearSymbolIDValues()
+    {
+        // short circuit exit - no SID's present to remove - so can exit immediately
+        if (!_isSymbolIdPresent())
+        {
+            return true;
+        }
+
+        boolean allSIDsRemoved = attemptClearSymbolIDValues();
+        // if all the SID's have been successfully removed - the SID Present flag can be set to false
+        if (allSIDsRemoved)
+        {
+            // clear the symbolID status flag
+            _isSymbolIdPresent(false);
+        }
+        return allSIDsRemoved;
+    }
+
     /**
      * Sets this value's symbol table to null, and erases any SIDs here and
      * recursively.
+     *
+     * @return true if all SID's have been successfully removed, otherwise false
      */
-    void clearSymbolIDValues()
+    boolean attemptClearSymbolIDValues()
     {
+        boolean sidsRemain = false;
         if (_fieldName != null)
         {
             _fieldId = UNKNOWN_SYMBOL_ID;
+        } else if (_fieldId > UNKNOWN_SYMBOL_ID)
+        {
+            // retaining the field SID, as it couldn't be cleared due to loss of context
+            // TODO - for SID handling consistency; this should attempt resolution first
+            sidsRemain = true;
         }
 
         if (_annotations != null)
@@ -496,6 +552,7 @@ abstract class IonValueLite
                 if (annotation == null) break;
 
                 String text = annotation.getText();
+                // TODO - for SID handling consistency; this should attempt resolution first, not just drop entry
                 if (text != null && annotation.getSid() != UNKNOWN_SYMBOL_ID)
                 {
                     _annotations[i] =
@@ -503,8 +560,20 @@ abstract class IonValueLite
                 }
             }
         }
+
+        return !sidsRemain;
     }
 
+    protected void cascadeSIDPresentToContextRoot() {
+        // start with self
+        IonValueLite node = this;
+        // iterate from leaf to context root setting encoding present until either context root is hit OR a node is
+        // encountered that already has encodings present (and so no further propogation is required).
+        while (null != node && !node._isSymbolIdPresent()) {
+            node._isSymbolIdPresent(true);
+            node = node.getContainer();
+        }
+    }
 
     final void setFieldName(String name)
     {
@@ -525,6 +594,12 @@ abstract class IonValueLite
         assert _fieldId == UNKNOWN_SYMBOL_ID && _fieldName == null;
         _fieldName = name.getText();
         _fieldId   = name.getSid();
+
+        // if a SID has been added by this operation to a previously SID-less node we have to mark upwards
+        // towards the context root that a SID is present
+        if (UNKNOWN_SYMBOL_ID != _fieldId && !_isSymbolIdPresent()) {
+            cascadeSIDPresentToContextRoot();
+        }
     }
 
     public final String getFieldName()
@@ -625,6 +700,19 @@ abstract class IonValueLite
         else
         {
             _annotations = annotations.clone();
+
+            // new annotations could have SID's - so if this node is not currently marked as SID
+            // present then the new annotations have to be interrogated to see if they contain SID's and if they
+            // do - the SID Present flag must be cascaded.
+            if (!_isSymbolIdPresent()) {
+                for (SymbolToken token : _annotations) {
+                    // upon finding first match of a symbol token containing a SID - cascade upwards and exit
+                    if (null != token && UNKNOWN_SYMBOL_ID != token.getSid()) {
+                        cascadeSIDPresentToContextRoot();
+                        break;
+                    }
+                }
+            }
         }
     }
 


Test patch:
diff --git a/test/AllTests.java b/test/AllTests.java
index b5b44ef2..609b86e8 100644
--- a/test/AllTests.java
+++ b/test/AllTests.java
@@ -71,6 +71,7 @@ import software.amazon.ion.impl.bin.IonRawBinaryWriterTest;
 import software.amazon.ion.impl.bin.PooledBlockAllocatorProviderTest;
 import software.amazon.ion.impl.bin.WriteBufferTest;
 import software.amazon.ion.impl.lite.IonContextTest;
+import software.amazon.ion.impl.lite.SIDPresentLifecycleTest;
 import software.amazon.ion.streaming.BadIonStreamingTest;
 import software.amazon.ion.streaming.BinaryStreamingTest;
 import software.amazon.ion.streaming.GoodIonStreamingTest;
@@ -198,6 +199,10 @@ import software.amazon.ion.util.TextTest;
     // Hash code tests
     HashCodeCorrectnessTest.class,
     HashCodeDistributionTest.class,
+    HashCodeDeltaCollisionTest.class,
+
+    // DOM Lifecycle / mode tests
+    SIDPresentLifecycleTest.class,
     HashCodeDeltaCollisionTest.class
 })
 public class AllTests
diff --git a/test/software/amazon/ion/impl/lite/SIDPresentLifecycleTest.java b/test/software/amazon/ion/impl/lite/SIDPresentLifecycleTest.java
new file mode 100644
index 00000000..d62e8f89
--- /dev/null
+++ b/test/software/amazon/ion/impl/lite/SIDPresentLifecycleTest.java
@@ -0,0 +1,224 @@
+package software.amazon.ion.impl.lite;
+
+import software.amazon.ion.IonTestCase;
+import software.amazon.ion.IonValue;
+import software.amazon.ion.SymbolTable;
+import software.amazon.ion.UnknownSymbolException;
+import software.amazon.ion.impl.PrivateUtils;
+import org.junit.Test;
+
+/**
+ * Tests to ensure that the current emerged retention behaviors for SID's across the IonValueLite DOM are correctly
+ * preserved with the symbol ID flag / state management. As of time of authoring the observed behavior for SID
+ * management is as follows:
+ *
+ * -------------------------------------------------------------------------------------------------------
+ * |                     | fieldName                 | annotations               | value (IonSymbolLite) |
+ * |                     | text + SID | SID only     | text + SID | SID only     | text + SID | SID only |
+ * -------------------------------------------------------------------------------------------------------
+ * | <init>              | N/A        | N/A          | N/A        | N/A          | clear      | retain   |
+ * | clone               | clear      | resolve/fail | clear      | retain       | clear      | clear (?)|
+ * | clearSymbolIDValues | clear      | retain       | clear      | clear [null] | clear      | retain   |
+ * | set                 | retain     | retain       | retain     | retain       | N/A        | N/A      |
+ * | get                 | ephemeral  | ephemeral    | ephemeral  | N/A          | ephemeral  | memoizes |
+ * -------------------------------------------------------------------------------------------------------
+ *
+ */
+public class SIDPresentLifecycleTest extends IonTestCase {
+
+    @Test
+    public void testFieldsWithSID() {
+        IonStringLite value = (IonStringLite) system().newString("Test Value");
+        // a new value should create without SID presence as it has no annotations
+        assertFalse(value._isSymbolIdPresent());
+        value.setFieldNameSymbol(PrivateUtils.newSymbolToken("Symbol-With-Sid", 12));
+        // setting a field with a SID means that SID Present now must be true
+        assertTrue(value._isSymbolIdPresent());
+
+        // test clone of the value results in no SID present
+        assertFalse(value.clone()._isSymbolIdPresent());
+
+        // verify detaching from the container clears the SID present flag
+        value.detachFromContainer();
+        assertFalse(value._isSymbolIdPresent());
+
+        //verify that detaching still retains symbols - as SID is still present
+        value.setFieldNameSymbol(PrivateUtils.newSymbolToken(13));
+        assertTrue(value._isSymbolIdPresent());
+        value.clearSymbolIDValues();
+        // NOTE: SID is still present as clearSymbolIDValues doesn't clear SID-only tokens
+        assertEquals(13, value.getFieldNameSymbol().getSid());
+        assertTrue(value._isSymbolIdPresent());
+    }
+
+    @Test
+    public void testAnnotationsWithSIDs() {
+        IonStringLite value = (IonStringLite) system().newString("Test Value 2");
+        // a new value should create without any SID's as it has no annotations
+        assertFalse(value._isSymbolIdPresent());
+        value.setTypeAnnotationSymbols(
+            PrivateUtils.newSymbolToken("Symbol-Without-SID", -1),
+            PrivateUtils.newSymbolToken("Symbol-With-SID", 99));
+        // setting an annotation with a SID means that SID present now must be true
+        assertTrue(value._isSymbolIdPresent());
+
+        // test clone of the value (given no *unresolvable* SID's) results in no SID's present
+        assertFalse(value.clone()._isSymbolIdPresent());
+
+        // ensure that setting annotations that don't have SIDs doesn't change the SID present status (need to wait for
+        // an explicit action such as detatch from container or clone).
+        value.setTypeAnnotationSymbols(PrivateUtils.newSymbolToken("Symbol-Without-Sid", -1));
+        assertTrue(value._isSymbolIdPresent());
+
+        // verify detaching from the container clears the SID Present flag
+        value.detachFromContainer();
+        assertFalse(value._isSymbolIdPresent());
+
+        // verify that where SID context preserves through the clone due to SID context not being known (believed a bug)
+        // that the SID present flag is *retained*
+        value.setTypeAnnotationSymbols(PrivateUtils.newSymbolToken(101));
+        assertTrue(value.clone()._isSymbolIdPresent());
+    }
+
+    @Test
+    public void testPropagationAcrossGraph() {
+        IonStructLite struct = (IonStructLite) system().newEmptyStruct();
+        assertFalse(struct._isSymbolIdPresent());
+
+        // 1. test addition of child by field WITHOUT SID causes no propagation
+        IonStringLite keyValue1 = (IonStringLite) system().newString("Foo");
+        struct.add(PrivateUtils.newSymbolToken("field_1", -1), keyValue1);
+        assertFalse(struct._isSymbolIdPresent());
+        assertFalse(keyValue1._isSymbolIdPresent());
+
+        // 2. test addition of child by field WITH SID but also with field text causes no propagation
+        // (struct only takes field name if present and strips SID)
+        IonStringLite keyValue2 = (IonStringLite) system().newString("Bar");
+        struct.add(PrivateUtils.newSymbolToken("field_2", 87), keyValue2);
+        assertFalse(struct._isSymbolIdPresent());
+        assertFalse(keyValue1._isSymbolIdPresent());
+        assertFalse(keyValue2._isSymbolIdPresent());
+
+        // 3. test addition of child by field with SID ONLY causes propagation due to retention of field SID
+        IonStringLite keyValue3 = (IonStringLite) system().newString("Car");
+        struct.add(PrivateUtils.newSymbolToken(76), keyValue3);
+        assertTrue(struct._isSymbolIdPresent());
+        assertFalse(keyValue1._isSymbolIdPresent());
+        assertFalse(keyValue2._isSymbolIdPresent());
+        assertTrue(keyValue3._isSymbolIdPresent());
+
+        // 4. test explicitly annotating a field propagates
+        IonStructLite struct2 = (IonStructLite) system().newEmptyStruct();
+        IonStringLite keyValue4 = (IonStringLite) system().newString("But");
+        struct2.add("field_1", keyValue4);
+        keyValue4.setTypeAnnotationSymbols(PrivateUtils.newSymbolToken("Lah", 13));
+        assertTrue(struct2._isSymbolIdPresent());
+
+        // 5. test clone propagates clearing of status. NOTE - clone with an unresolvable SID for a field ID fails
+        try {
+            struct.clone();
+            fail("clone was expected NOT to succeed due to behavior at time of writing where unresolvable SID's fail");
+        } catch (UnknownSymbolException use) {
+            // this is expected??! until someone fixed the TO DO in IonValueLite#getFieldName
+        }
+
+        // remove the field without SID such that the clone can be enacted.
+        struct.remove_child(2);
+
+        IonStructLite clonedStruct = struct.clone();
+        assertFalse(clonedStruct._isSymbolIdPresent());
+        for (IonValue value : clonedStruct) {
+            assertFalse(((IonValueLite) value)._isSymbolIdPresent());
+        }
+
+        // 6. ensure clearing symbols propogates from root to impacted leaves
+        // add back in field ID (SID) only child before clearing to test SID retained
+        struct.add(PrivateUtils.newSymbolToken(76), keyValue3);
+        struct.clearSymbolIDValues();
+        assertTrue(struct._isSymbolIdPresent());
+        assertFalse(keyValue1._isSymbolIdPresent());
+        assertFalse(keyValue2._isSymbolIdPresent());
+        assertTrue(keyValue3._isSymbolIdPresent());
+
+        // 7. ensure that nested SID-only annotation clone behavior propogates to cloned root.
+        IonStructLite outer = (IonStructLite) system().newEmptyStruct();
+        IonListLite middle = (IonListLite) system().newEmptyList();
+        IonIntLite inner = (IonIntLite) system().newInt(10);
+        middle.add(inner);
+        outer.put("foo", middle);
+        // now add a SID only annotation to inner
+        inner.setTypeAnnotationSymbols(PrivateUtils.newSymbolToken(99));
+        // verify that all components are signaling SID present
+        assertTrue(outer._isSymbolIdPresent());
+        assertTrue(middle._isSymbolIdPresent());
+        assertTrue(inner._isSymbolIdPresent());
+        // conduct a clone and verify all cloned components have retained SID
+        IonStructLite clonedOuter = outer.clone();
+        assertTrue(clonedOuter._isSymbolIdPresent());
+        IonListLite clonedMiddle = (IonListLite) outer.get("foo");
+        assertTrue(clonedMiddle._isSymbolIdPresent());
+        assertTrue(clonedMiddle.get_child(0)._isSymbolIdPresent());
+    }
+
+    @Test
+    public void testSymbolValueLifecyle() {
+        // test creation from SymbolToken
+        IonSymbolLite symbolLite = (IonSymbolLite) system().newSymbol(PrivateUtils.newSymbolToken("version", 5));
+        // SID isn't present as it is stripped if there is text present
+        assertFalse(symbolLite._isSymbolIdPresent());
+
+        IonListLite container1 = (IonListLite) system().newEmptyList();
+        container1.add(symbolLite);
+        // test propagation of context when symbol context is added
+        assertEquals(5, symbolLite.symbolValue().getSid());
+        // as the getSymbolId memoizes SID into the entity SID present must be set
+        assertTrue(symbolLite._isSymbolIdPresent());
+        // check memoization propagates to container
+        assertTrue(container1._isSymbolIdPresent());
+        container1.clearSymbolIDValues();
+        assertFalse(container1._isSymbolIdPresent());
+        assertFalse(symbolLite._isSymbolIdPresent());
+
+        IonSymbolLite symbolSIDOnly = (IonSymbolLite) system().newSymbol(PrivateUtils.newSymbolToken(321));
+        // SID Present is preserved when only SID is present
+        assertTrue(symbolSIDOnly._isSymbolIdPresent());
+        IonListLite container2 = (IonListLite) system().newEmptyList();
+        container2.add(symbolSIDOnly);
+        assertTrue(container2._isSymbolIdPresent());
+        // clear symbol ID values won't clear the symbolSIDOnly SID value - therefore container2 must still remain
+        // marked as having a SID present
+        container2.clearSymbolIDValues();
+        assertTrue(container2._isSymbolIdPresent());
+        assertTrue(symbolSIDOnly._isSymbolIdPresent());
+    }
+
+    @Test
+    public void testSIDRetentionAcrossObjectHierarchy() {
+        // field ID's can be retained on Container - so ensure these are retained.
+        IonStructLite outer = (IonStructLite) system().newEmptyStruct();
+        IonListLite middle = (IonListLite) system().newEmptyList();
+        IonStringLite inner = (IonStringLite) system().newString("A");
+        outer.add(PrivateUtils.newSymbolToken(321), middle);
+        middle.add(inner);
+        inner.setTypeAnnotationSymbols(PrivateUtils.newSymbolToken("B", 123));
+        assertTrue(outer._isSymbolIdPresent());
+        assertTrue(middle._isSymbolIdPresent());
+        assertTrue(inner._isSymbolIdPresent());
+        outer.clearSymbolIDValues();
+        assertTrue(outer._isSymbolIdPresent());
+        assertTrue(middle._isSymbolIdPresent());
+        assertFalse(inner._isSymbolIdPresent());
+
+        // field ID's can be retained on IonSymbolLite - so ensure these are retained as well.
+        IonStructLite container = (IonStructLite) system().newEmptyStruct();
+        IonSymbolLite symbol = (IonSymbolLite) system().newSymbol(PrivateUtils.newSymbolToken("Foo", 123));
+        container.add(PrivateUtils.newSymbolToken(321), symbol);
+        container.clearSymbolIDValues();
+        assertEquals(SymbolTable.UNKNOWN_SYMBOL_ID, symbol.symbolValue().getSid());
+        assertEquals("Foo", symbol.stringValue());
+        assertEquals(321, symbol.getFieldNameSymbol().getSid());
+        assertTrue(symbol._isSymbolIdPresent());
+
+        assertTrue(container._isSymbolIdPresent());
+    }
+}