Repository: amzn/ion-java (java)

Issue:
Support Writing Local Symbol Table Append
Read support was introduced in #98 still missing write support

Pull request description:
Adds support for writing LST append after IonWriter.flush()
*Issue #, if available:*
Fixes #178 ; supersedes PR #236 

*Description of changes:*
Now, `IonWriter.flush()` always flushes data when at the top level. Subsequent values that contain symbols will initiate an appended LST. The behavior of `IonWriter.finish()` is unchanged.

The writer also supports intercepting user calls that manually construct a symbol table. LST append support has been added for that case as well.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


Patch:
diff --git a/src/com/amazon/ion/IonWriter.java b/src/com/amazon/ion/IonWriter.java
index fe1e0e96..dae9c08e 100644
--- a/src/com/amazon/ion/IonWriter.java
+++ b/src/com/amazon/ion/IonWriter.java
@@ -58,12 +58,12 @@ import java.util.Date;
  * <p>
  * Once all the top-level values have been written (and stepped-out back to
  * the starting level), the caller must {@link #close()} the writer
- * (or at least {@link #finish()} it) before accessing
+ * (or at least {@link #flush()} or {@link #finish()} it) before accessing
  * the data written to the underlying data sink
  * (for example, via {@link ByteArrayOutputStream#toByteArray()}).
- * The writer may have internal buffers and without closing or finishing it,
- * it may not have written everything to the underlying data sink. In addition,
- * {@link #flush()} isn't guaranteed to be sufficient for all implementations.
+ * The writer may have internal buffers and without closing, flushing, or
+ * finishing it, it may not have written everything to the underlying data
+ * sink.
  *
  * <h2>Exception Handling</h2>
  * {@code IonWriter} is a generic interface for generating Ion data, and it's
@@ -106,18 +106,19 @@ public interface IonWriter
 
     /**
      * Flushes this writer by writing any buffered output to the underlying
-     * output target.
+     * output target without finalizing the stream's local symbol table.
      * <p>
      * For some implementations this may have no effect even when some data is
      * buffered, because it's not always possible to fully write partial data.
      * In particular, when writing binary Ion data, Ion's length-prefixed
      * encoding requires a complete top-level value to be written at once.
      * <p>
-     * Furthermore, when writing binary Ion data, <em>nothing</em> can be
-     * flushed until the writer knows that no more local symbols can be
-     * encountered. This can be accomplished via {@link #finish()} or by
-     * making the {@linkplain #getSymbolTable() local symbol table}
-     * {@linkplain SymbolTable#makeReadOnly() read-only}.
+     * This feature can be used to flush buffered data before writing more
+     * values without subsequently having to redeclare the current local
+     * symbol table. Applications that produce long streams of binary Ion may
+     * wish to flush occasionally to relieve memory pressure, then continue
+     * writing data using the same local symbol table. The symbol table will
+     * be appended with newly-encountered symbols as necessary.
      *
      * @throws IOException if thrown by the underlying output target.
      *
@@ -144,13 +145,16 @@ public interface IonWriter
      * {@link com.amazon.ion.system.IonWriterBuilder.IvmMinimizing
      * IvmMinimizing}.)
      * <p>
-     * This feature can be used to flush reliably before writing more values.
-     * Think about a long-running stream of binary values: without finishing,
-     * all the values would continue to buffer since the encoder keeps
-     * expecting more local symbols (which must be written into the local
-     * symbol table that precedes all top-level values). Such an application
-     * can finish occasionally to flush the data out, then continue writing
-     * more data using a fresh local symbol table.
+     * This feature can be used to flush buffered data and reset the local
+     * symbol table before writing more values. Applications that produce long
+     * streams of binary Ion may wish to finish occasionally to relieve memory
+     * pressure, then continue writing data using a new local symbol table.
+     * This is particularly useful for streams that contain an ever-growing
+     * number of unique symbols to avoid unbounded growth of the symbol table,
+     * which may degrade performance and bloat the encoding. Applications that
+     * produce long streams with a limited number of unique symbols should
+     * use {@link #flush()} instead to avoid re-declaring the local symbol
+     * table unnecessarily.
      *
      * @throws IOException if thrown by the underlying output target.
      * @throws IllegalStateException when not between top-level values.
diff --git a/src/com/amazon/ion/impl/bin/IonManagedBinaryWriter.java b/src/com/amazon/ion/impl/bin/IonManagedBinaryWriter.java
index e898f2a1..2299baf2 100644
--- a/src/com/amazon/ion/impl/bin/IonManagedBinaryWriter.java
+++ b/src/com/amazon/ion/impl/bin/IonManagedBinaryWriter.java
@@ -368,12 +368,20 @@ import java.util.Map;
                     // since we don't know what's coming after (i.e. new local symbols)
                     self.user.truncate(self.userSymbolTablePosition);
 
-                    // flush out the pre-existing symbol and user content before the user provided symbol table
-                    self.finish();
+                    if (self.isUserLSTAppend)
+                    {
+                        self.flush();
+                    }
+                    else
+                    {
+
+                        // flush out the pre-existing symbol and user content before the user provided symbol table
+                        self.finish();
 
-                    // replace the symbol table context with the user provided one
-                    // TODO determine if the resolver mode should be configurable for this use case
-                    self.imports = new ImportedSymbolContext(ImportedSymbolResolverMode.DELEGATE, self.userImports);
+                        // replace the symbol table context with the user provided one
+                        // TODO determine if the resolver mode should be configurable for this use case
+                        self.imports = new ImportedSymbolContext(ImportedSymbolResolverMode.DELEGATE, self.userImports);
+                    }
 
                     // explicitly start the local symbol table with no version marker
                     // in case we need the previous symbols
@@ -392,10 +400,24 @@ import java.util.Map;
                     self.userCurrentImport.reset();
                     self.userImports.clear();
                     self.userSymbols.clear();
+                    self.isUserLSTAppend = false;
 
                     self.userState = NORMAL;
                 }
             }
+
+            @Override
+            public void writeSymbolToken(final IonManagedBinaryWriter self, final SymbolToken value)
+            {
+                if (
+                    self.user.getDepth() == 1
+                    && self.user.getFieldId() == IMPORTS_SID
+                    && value.getSid() == ION_SYMBOL_TABLE_SID
+                ) {
+                    self.isUserLSTAppend = true;
+                    self.userState = LOCALS_AT_TOP;
+                }
+            }
         },
         LOCALS_AT_IMPORTS
         {
@@ -515,6 +537,7 @@ import java.util.Map;
         public abstract void afterStepOut(final IonManagedBinaryWriter self) throws IOException;
 
         public void writeString(final IonManagedBinaryWriter self, final String value) throws IOException {}
+        public void writeSymbolToken(final IonManagedBinaryWriter self, final SymbolToken value) {}
         public void writeInt(final IonManagedBinaryWriter self, final long value) throws IOException {}
         public void writeInt(IonManagedBinaryWriter self, BigInteger value) throws IOException
         {
@@ -636,6 +659,7 @@ import java.util.Map;
     private final List<SymbolTable>             userImports;
     private final List<String>                  userSymbols;
     private final ImportDescriptor              userCurrentImport;
+    private boolean                             isUserLSTAppend;
 
     private boolean                             closed;
 
@@ -680,6 +704,7 @@ import java.util.Map;
         this.userImports = new ArrayList<SymbolTable>();
         this.userSymbols = new ArrayList<String>();
         this.userCurrentImport = new ImportDescriptor();
+        this.isUserLSTAppend = false;
 
         // TODO decide if initial LST should survive finish() and seed the next LST
         final SymbolTable lst = builder.initialSymbolTable;
@@ -740,16 +765,22 @@ import java.util.Map;
 
     private void startLocalSymbolTableIfNeeded(final boolean writeIVM) throws IOException
     {
-        if (symbolState == SymbolState.SYSTEM_SYMBOLS)
+        boolean isAppend = symbolState == SymbolState.LOCAL_SYMBOLS_FLUSHED;
+        if (symbolState == SymbolState.SYSTEM_SYMBOLS || isAppend)
         {
-            if (writeIVM)
+            if (writeIVM && !isAppend)
             {
                 symbols.writeIonVersionMarker();
             }
             symbols.addTypeAnnotationSymbol(systemSymbol(ION_SYMBOL_TABLE_SID));
             symbols.stepIn(STRUCT);
             {
-                if (imports.parents.size() > 0)
+                if (isAppend)
+                {
+                    symbols.setFieldNameSymbol(systemSymbol(IMPORTS_SID));
+                    symbols.writeSymbolToken(systemSymbol(ION_SYMBOL_TABLE_SID));
+                }
+                else if (imports.parents.size() > 0)
                 {
                     symbols.setFieldNameSymbol(systemSymbol(IMPORTS_SID));
                     symbols.stepIn(LIST);
@@ -1025,6 +1056,7 @@ import java.util.Map;
             return;
         }
         token = intern(token);
+        userState.writeSymbolToken(this, token);
         user.writeSymbolToken(token);
     }
 
@@ -1071,7 +1103,7 @@ import java.util.Map;
 
     public void flush() throws IOException
     {
-        if (getDepth() == 0 && localsLocked)
+        if (getDepth() == 0 && !user.hasAnnotations())
         {
             unsafeFlush();
         }
@@ -1083,10 +1115,9 @@ import java.util.Map;
         {
             // this implies that we have a local symbol table of some sort and the user locked it
             symbolState.closeTable(symbols);
+            // make sure that until the local symbol state changes we no-op the table closing routine
+            symbolState = SymbolState.LOCAL_SYMBOLS_FLUSHED;
         }
-
-        // make sure that until the local symbol state changes we no-op the table closing routine
-        symbolState = SymbolState.LOCAL_SYMBOLS_FLUSHED;
         // push the data out
         symbols.finish();
         user.finish();


Test patch:
diff --git a/test/com/amazon/ion/impl/BinaryWriterTest.java b/test/com/amazon/ion/impl/BinaryWriterTest.java
index f2d28785..7110e152 100644
--- a/test/com/amazon/ion/impl/BinaryWriterTest.java
+++ b/test/com/amazon/ion/impl/BinaryWriterTest.java
@@ -15,6 +15,7 @@
 
 package com.amazon.ion.impl;
 
+import com.amazon.ion.IonCatalog;
 import com.amazon.ion.IonException;
 import com.amazon.ion.IonReader;
 import com.amazon.ion.IonType;
@@ -26,6 +27,9 @@ import com.amazon.ion.junit.IonAssert;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
+
+import com.amazon.ion.system.IonReaderBuilder;
+import com.amazon.ion.system.SimpleCatalog;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -120,27 +124,36 @@ public class BinaryWriterTest
     throws Exception
     {
         iw = makeWriter();
-        iw.writeSymbol("force a local symtab"); // TODO amzn/ion-java/issues/8
+        iw.writeSymbol("force a local symtab");
         SymbolTable symtab = iw.getSymbolTable();
         symtab.intern("fred_1");
         symtab.intern("fred_2");
 
         iw.writeSymbol("fred_1");
+        // This would cause an appended LST to be written before the next value.
         iw.flush();
-        byte[] bytes = myOutputStream.toByteArray();
-        assertEquals(0, bytes.length);
+        IonReader reader = IonReaderBuilder.standard().build(myOutputStream.toByteArray());
+        assertEquals(IonType.SYMBOL, reader.next());
+        assertEquals("force a local symtab", reader.stringValue());
+        assertEquals(IonType.SYMBOL, reader.next());
+        assertEquals("fred_1", reader.stringValue());
+        assertNull(reader.next());
     }
 
     @Test
     public void testFlushingUnlockedSymtabWithImports()
     throws Exception
     {
-        SymbolTable fred1 = Symtabs.register("fred",   1, catalog());
+        SimpleCatalog catalog = catalog();
+        SymbolTable fred1 = Symtabs.register("fred",   1, catalog);
         iw = makeWriter(fred1);
         iw.writeSymbol("fred_1");
+        // This would cause an appended LST to be written before the next value.
         iw.flush();
-        byte[] bytes = myOutputStream.toByteArray();
-        assertEquals(0, bytes.length);
+        IonReader reader = IonReaderBuilder.standard().withCatalog(catalog).build(myOutputStream.toByteArray());
+        assertEquals(IonType.SYMBOL, reader.next());
+        assertEquals("fred_1", reader.stringValue());
+        assertNull(reader.next());
     }
 
     @Test
diff --git a/test/com/amazon/ion/impl/IonWriterTestCase.java b/test/com/amazon/ion/impl/IonWriterTestCase.java
index e13ff69d..505a756e 100644
--- a/test/com/amazon/ion/impl/IonWriterTestCase.java
+++ b/test/com/amazon/ion/impl/IonWriterTestCase.java
@@ -47,6 +47,7 @@ import com.amazon.ion.SymbolToken;
 import com.amazon.ion.SystemSymbols;
 import com.amazon.ion.TestUtils;
 import com.amazon.ion.UnknownSymbolException;
+import com.amazon.ion.impl.bin._Private_IonManagedWriter;
 import com.amazon.ion.junit.IonAssert;
 import com.amazon.ion.system.IonSystemBuilder;
 import java.io.ByteArrayInputStream;
@@ -805,9 +806,24 @@ public abstract class IonWriterTestCase
         iw.writeSymbol("now");
         iw.close();
 
-        // Should have:  IVM SYMTAB hey now
         IonDatagram dg = reload();
-        assertEquals(4, dg.systemSize());
+        if (myOutputForm == OutputForm.BINARY && iw instanceof _Private_IonManagedWriter) {
+            // Note: LST append will only be implemented in the "new" binary writer (which implements
+            // _Private_IonMangedWriter)
+            // Should have:  IVM SYMTAB hey SYMTAB_APPEND now
+            assertEquals(5, dg.systemSize());
+            assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(1)).getTypeAnnotations()[0]);
+            assertEquals("hey", ((IonSymbol) dg.systemGet(2)).stringValue());
+            assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(3)).getTypeAnnotations()[0]);
+            assertEquals("now", ((IonSymbol) dg.systemGet(4)).stringValue());
+        }
+        else {
+            // Should have:  IVM SYMTAB hey SYMTAB_APPEND now
+            assertEquals(4, dg.systemSize());
+            assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(1)).getTypeAnnotations()[0]);
+            assertEquals("hey", ((IonSymbol) dg.systemGet(2)).stringValue());
+            assertEquals("now", ((IonSymbol) dg.systemGet(3)).stringValue());
+        }
     }
 
     Iterator<IonValue> systemIterateOutput()
diff --git a/test/com/amazon/ion/impl/OutputStreamWriterTestCase.java b/test/com/amazon/ion/impl/OutputStreamWriterTestCase.java
index acc7c010..4093ae4b 100644
--- a/test/com/amazon/ion/impl/OutputStreamWriterTestCase.java
+++ b/test/com/amazon/ion/impl/OutputStreamWriterTestCase.java
@@ -179,7 +179,6 @@ public abstract class OutputStreamWriterTestCase
         // Try flushing when there's just a pending annotation.
         iw.addTypeAnnotation("fred_1");
         iw.flush();
-        checkFlushed(true);
         myOutputStreamWrapper.flushed = false;
 
         bytes = myOutputStream.toByteArray();
diff --git a/test/com/amazon/ion/impl/bin/IonManagedBinaryWriterTest.java b/test/com/amazon/ion/impl/bin/IonManagedBinaryWriterTest.java
index b83bbbd2..416abc96 100644
--- a/test/com/amazon/ion/impl/bin/IonManagedBinaryWriterTest.java
+++ b/test/com/amazon/ion/impl/bin/IonManagedBinaryWriterTest.java
@@ -20,7 +20,11 @@ import static java.util.Collections.unmodifiableList;
 import static java.util.Collections.unmodifiableMap;
 
 import com.amazon.ion.IonContainer;
+import com.amazon.ion.IonDatagram;
+import com.amazon.ion.IonInt;
 import com.amazon.ion.IonMutableCatalog;
+import com.amazon.ion.IonReader;
+import com.amazon.ion.IonStruct;
 import com.amazon.ion.IonSymbol;
 import com.amazon.ion.IonType;
 import com.amazon.ion.IonValue;
@@ -179,6 +183,134 @@ public class IonManagedBinaryWriterTest extends IonRawBinaryWriterTest
         assertValue("name");
     }
 
+    @Test
+    public void testLocalSymbolTableAppend() throws Exception
+    {
+        writer.writeSymbol("taco");
+        writer.flush();
+        writer.writeSymbol("burrito");
+        writer.finish();
+        IonReader reader = system().newReader(writer.getBytes());
+        reader.next();
+        assertEquals(reader.getSymbolTable().findSymbol("taco"), 15);
+        assertEquals(reader.getSymbolTable().findSymbol("burrito"), -1);
+        reader.next();
+        assertEquals(reader.getSymbolTable().findSymbol("taco"), 15);
+        assertEquals(reader.getSymbolTable().findSymbol("burrito"), 16);
+        assertNull(reader.next());
+
+        IonDatagram dg = system().getLoader().load(writer.getBytes());
+        // Should be IVM SYMTAB taco SYMTAB burrito
+        assertEquals(5, dg.systemSize());
+        assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(1)).getTypeAnnotations()[0]);
+        assertEquals("taco", ((IonSymbol) dg.systemGet(2)).stringValue());
+        assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(3)).getTypeAnnotations()[0]);
+        assertEquals("burrito", ((IonSymbol) dg.systemGet(4)).stringValue());
+    }
+
+    @Test
+    public void testFlushImmediatelyAfterIVM() throws Exception
+    {
+        writer.flush();
+        writer.writeSymbol("burrito");
+        writer.finish();
+        IonReader reader = system().newReader(writer.getBytes());
+        reader.next();
+        assertEquals(reader.getSymbolTable().findSymbol("taco"), -1);
+        assertEquals(reader.getSymbolTable().findSymbol("burrito"), 15);
+        assertNull(reader.next());
+
+        IonDatagram dg = system().getLoader().load(writer.getBytes());
+        // Should be IVM SYMTAB burrito
+        assertEquals(3, dg.systemSize());
+        assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(1)).getTypeAnnotations()[0]);
+        assertEquals("burrito", ((IonSymbol) dg.systemGet(2)).stringValue());
+    }
+
+    @Test
+    public void testNoNewSymbolsAfterFlush() throws Exception
+    {
+        writer.writeSymbol("taco");
+        writer.flush();
+        writer.writeInt(123);
+        writer.finish();
+
+        IonDatagram dg = system().getLoader().load(writer.getBytes());
+        // Should be IVM SYMTAB taco 123
+        assertEquals(4, dg.systemSize());
+        assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(1)).getTypeAnnotations()[0]);
+        assertEquals("taco", ((IonSymbol) dg.systemGet(2)).stringValue());
+        assertEquals(123, ((IonInt) dg.systemGet(3)).intValue());
+    }
+
+    @Test
+    public void testManuallyWriteLSTAppendWithImportsFirst() throws Exception
+    {
+        writer.writeSymbol("taco");
+        writer.addTypeAnnotation("$ion_symbol_table");
+        writer.stepIn(IonType.STRUCT);
+        writer.setFieldName("imports");
+        writer.writeSymbol("$ion_symbol_table");
+        writer.setFieldName("symbols");
+        writer.stepIn(IonType.LIST);
+        writer.writeString("burrito");
+        writer.stepOut();
+        writer.stepOut();
+        writer.writeSymbol("burrito");
+        writer.finish();
+
+        IonDatagram dg = system().getLoader().load(writer.getBytes());
+        // Should be IVM SYMTAB taco SYMTAB burrito
+        assertEquals(5, dg.systemSize());
+        assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(1)).getTypeAnnotations()[0]);
+        assertEquals("taco", ((IonSymbol) dg.systemGet(2)).stringValue());
+        assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(3)).getTypeAnnotations()[0]);
+        assertEquals("burrito", ((IonSymbol) dg.systemGet(4)).stringValue());
+
+        IonReader reader = system().newReader(writer.getBytes());
+        reader.next();
+        assertEquals(reader.getSymbolTable().findSymbol("taco"), 15);
+        assertEquals(reader.getSymbolTable().findSymbol("burrito"), -1);
+        reader.next();
+        assertEquals(reader.getSymbolTable().findSymbol("taco"), 15);
+        assertEquals(reader.getSymbolTable().findSymbol("burrito"), 16);
+        assertNull(reader.next());
+    }
+
+    @Test
+    public void testManuallyWriteLSTAppendWithSymbolsFirst() throws Exception
+    {
+        writer.writeSymbol("taco");
+        writer.addTypeAnnotation("$ion_symbol_table");
+        writer.stepIn(IonType.STRUCT);
+        writer.setFieldName("symbols");
+        writer.stepIn(IonType.LIST);
+        writer.writeString("burrito");
+        writer.stepOut();
+        writer.setFieldName("imports");
+        writer.writeSymbol("$ion_symbol_table");
+        writer.stepOut();
+        writer.writeSymbol("burrito");
+        writer.finish();
+
+        IonDatagram dg = system().getLoader().load(writer.getBytes());
+        // Should be IVM SYMTAB taco SYMTAB burrito
+        assertEquals(5, dg.systemSize());
+        assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(1)).getTypeAnnotations()[0]);
+        assertEquals("taco", ((IonSymbol) dg.systemGet(2)).stringValue());
+        assertEquals("$ion_symbol_table", ((IonStruct) dg.systemGet(3)).getTypeAnnotations()[0]);
+        assertEquals("burrito", ((IonSymbol) dg.systemGet(4)).stringValue());
+
+        IonReader reader = system().newReader(writer.getBytes());
+        reader.next();
+        assertEquals(reader.getSymbolTable().findSymbol("taco"), 15);
+        assertEquals(reader.getSymbolTable().findSymbol("burrito"), -1);
+        reader.next();
+        assertEquals(reader.getSymbolTable().findSymbol("taco"), 15);
+        assertEquals(reader.getSymbolTable().findSymbol("burrito"), 16);
+        assertNull(reader.next());
+    }
+
     @Test
     public void testUserFieldNames() throws Exception
     {