Add managed writer for Ion 1.1 with basic round-trip testing #830

popematt · 2024-05-03T22:36:29Z

Issue #, if available:
None

Description of changes:

This adds a "managed" writer for Ion 1.1 that is generic over whether the raw encoding is Text or Binary.

Caveats:

There are a lot of classes that need unit testing
I had to change the visibility of at least a half dozen existing classes. We will need to revisit the package organization and the visibility of those classes.
There is a lot of documentation that is not complete yet.
Does not support shared symbol tables.
...probably others? I've exhausted my mental capacity for the week, and I just need to get this out.

However, this does have some basic round-trip tests using the data in ion-tests and this is enough that we can start building other things around it an incrementally filling in the gaps.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

popematt · 2024-05-03T22:42:39Z

src/main/java/com/amazon/ion/impl/BufferedOutputStream.kt

+ * TODO:
+ *   - Add proper tests
+ */
+internal class BufferedOutputStream(


🗺️ This allows the text writer to be buffered, like the binary writer, so that we can make sure that we buffer enough before writing out the encoding directives. We still need to add some way of specifying parts of the encoding directives ahead of time so that we don't have to do any buffering. If we can guarantee that there will be no unexpected encoding directives, then we don't need to buffer the text writer.

popematt · 2024-05-03T22:46:10Z

src/main/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt

@@ -17,6 +17,7 @@ import java.time.Instant
 *  - Never writes using "long string" syntax in order to simplify the writer.
 *  - Uses `[: ... ]` for expression groups.
 *  - Does not try to resolve symbol tokens. That is the concern of the managed writer.
+ *  - To make it easier to concatenate streams, this eagerly emits a top-level separator after each top-level syntax item.


🗺️ I think this is something that we can probably live with.

src/main/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt

popematt · 2024-05-03T22:48:04Z

src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java

-                return sign == 0 ? -0e0 : 0e0;
+                return sign == 0 ? 0e0 : -0e0;


🗺️ I'm pretty sure that if I'm understanding it right, when the sign bit is 0, the float is positive. Either way, I needed to make this change to get the Ion 1.1 round trip tests to pass.

popematt · 2024-05-03T22:48:32Z

src/main/java/com/amazon/ion/impl/LocalSymbolTable.java

@@ -49,11 +36,11 @@
 * <p>
 * Instances of this class are safe for use by multiple threads.
 */
-class LocalSymbolTable
+public class LocalSymbolTable


🗺️ Just visibility changes in this file.

src/main/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt

src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt

src/main/java/com/amazon/ion/impl/bin/IonRawWriter_1_1_Shim.kt

tgregg

Let's open an issue with a list of the classes we had to make public in this PR so we don't forget to go back and revisit them.

Approving with comments, since we already plan to iterate on this.

tgregg · 2024-05-03T22:47:11Z

src/main/java/com/amazon/ion/impl/BufferedOutputStream.kt

+ * TODO:
+ *   - Add proper tests
+ */
+internal class BufferedOutputStream(


Can you comment a little more on why this is needed? Also, I suggest a name that doesn't clash with a built-in.

The OutputStreamFastAppendable buffers up to 4096 bytes before flushing to the output stream. If you have a text top-level value that is larger than 4096 bytes, then you can end up with partial or whole user values flushing to the OutputStream before you write the system value that it depends on.

I can change the name.

tgregg · 2024-05-03T22:53:47Z

src/main/java/com/amazon/ion/impl/bin/DelimitedContainerStrategy.kt

+     * TODO: See if we can add other context, such as annotations that are going to be added to this container,
+     *       or the field name (if this container is in a struct).


I wonder if there's a "WriterView" interface we could define that we could vend any desired context through.

fun writeDelimited(context: WriterView): Boolean

interface WriterView { ContainerType getContainerDepth(); int getDepth(); // etc. }

I'm also starting to think of the expense of this. Evaluating on every container might be prohibitive for more complicated rules. I wonder if we could somehow compile the rules into the writer so it can keep track of which options are active as it progresses in the stream, similar to how the path extractor keeps track of partial matches at its current location. If there are no potential matches, no invocations of the callback need be made.

I think we can go with something like what's proposed for the preview, and iterate once we have some performance numbers.

The WriterView is a good idea.

tgregg · 2024-05-03T23:10:04Z

src/main/java/com/amazon/ion/impl/bin/ManagedWriterOptions.kt

+ * Options that are specific to Ion 1.1 and handled in the managed writer.
+ * These are (mostly) generalizable to both text and binary.
+ */
+data class ManagedWriterOptions(


Should this have a _1_1 suffix?

tgregg · 2024-05-03T23:10:32Z

src/main/java/com/amazon/ion/impl/bin/ManagedWriterOptions.kt

+    /**
+     * Whether the symbols in the encoding directive should be interned or not.
+     * For binary, almost certainly want this to be true, and for text, it's
+     * more readable if it's false.
+     */
+    val internEncodingDirectiveSymbols: Boolean = false,


What would the text look like if this were true?

The symbol table would look something like this:

$3::{ $7: [ ... ] }

vs.

$ion_symbol_table::{ symbols: [ ... ] }

It's not super important, but when you're writing text with a symbol table, I think it help make it easier for a human to find and read the symbol table.

tgregg · 2024-05-03T23:14:41Z

src/main/java/com/amazon/ion/impl/bin/SymbolInliningStrategy.kt

+    /**
+     * Indicates whether a particular symbol text should be written inline (as opposed to writing as a SID).
+     */
+    fun shouldWriteInline(symbolKind: SymbolKind, text: String): Boolean
+


Are we going to call this on every field name, annotation, and symbol value?

It does right now. Now that I've been away from this for an hour, it does seem like that could be a performance problem. We can get rid of it if we need to.

tgregg · 2024-05-03T23:30:38Z

src/main/java/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt

+        if (content == null) {
+            userData.writeNull(IonType.SYMBOL)
+        } else {
+            handleSymbolText(content, options.shouldWriteInline(SymbolKind.VALUE, content), userData::writeSymbol, userData::writeSymbol)


Should the inlining decision for a particular symbol be cached?

Maybe? A HashMap lookup might be more expensive than a simple check such as text.length > 5.

But also, I think that caching the decision prevents some of the interesting implementation ideas that I mentioned. (E.g. only intern the symbol after it's occurred at least N times.)

As I've thought about this more, I think that caching works well if it is implemented as part of the SymbolInliningStrategy. This way, the SymbolInliningStrategy can manage the cache invalidation in a way that works with its logic.

Add managed writer for Ion 1.1 with basic round-trip testing

adb15fc

popematt requested a review from tgregg May 3, 2024 22:36

popematt commented May 3, 2024

View reviewed changes

Adds self-suggested changes from PR

2eaed4d

tgregg approved these changes May 3, 2024

View reviewed changes

popematt mentioned this pull request May 6, 2024

Revisit visibility changes and package locations of classes in #830 #832

Open

Adds changes based on PR feedback

97f3c26

tgregg approved these changes May 6, 2024

View reviewed changes

popematt merged commit 627ffb3 into amazon-ion:ion-11-encoding May 6, 2024
15 of 34 checks passed

popematt deleted the ion-11-managed-writer branch May 6, 2024 23:51

tgregg pushed a commit that referenced this pull request Jun 28, 2024

Add managed writer for Ion 1.1 with basic round-trip testing (#830)

85b03c1

tgregg pushed a commit that referenced this pull request Sep 9, 2024

Add managed writer for Ion 1.1 with basic round-trip testing (#830)

686f585

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add managed writer for Ion 1.1 with basic round-trip testing #830

Add managed writer for Ion 1.1 with basic round-trip testing #830

popematt commented May 3, 2024

popematt May 3, 2024

popematt May 3, 2024

popematt May 3, 2024

popematt May 3, 2024

tgregg left a comment

tgregg May 3, 2024

popematt May 3, 2024

tgregg May 3, 2024

popematt May 3, 2024

tgregg May 3, 2024

tgregg May 3, 2024

popematt May 3, 2024

tgregg May 3, 2024

popematt May 3, 2024

tgregg May 3, 2024

popematt May 3, 2024

popematt May 6, 2024

		return sign == 0 ? -0e0 : 0e0;
		return sign == 0 ? 0e0 : -0e0;

		* TODO: See if we can add other context, such as annotations that are going to be added to this container,
		* or the field name (if this container is in a struct).

Add managed writer for Ion 1.1 with basic round-trip testing #830

Add managed writer for Ion 1.1 with basic round-trip testing #830

Conversation

popematt commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgregg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment