IonRawBinaryWriter_1_1 annotations #661

popematt · 2023-12-01T23:06:45Z

Issue #, if available:

None

Description of changes:

There are two commits in this PR.

The first commit refactors some tests. It adds an autoClose option to the assertWriterOutputEquals function and an assertWriterThrows function, and it updates the test cases to use those.

The second commit adds annotations to IonRawBinaryWriter_1_1.

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

popematt · 2023-12-01T23:09:34Z

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

     * [writeAnnotations] may be called more than once to build up a list of annotations.
     */
-    fun writeAnnotations(annotation0: Int, annotation1: Int, vararg annotations: Int)
+    fun writeAnnotations(annotations: IntArray)


🗺️ I realized that it (a) made the implementation cleaner and (b) just made sense to be explicit about the array here, so I changed the function signature.

Does this ever need to be called from Java? What does that look like?

For now, I don't think the Ion 1.1 raw writer needs to be called from Java. However, it is callable from Java. IntArray is Kotlin's name for int[].

As for removing the vararg parameter—instead of this:

writer.writeAnnotations(1, 2, 3, 4, 5);

Now you have to be explicit about the array:

write.writeAnnotations(new int[]{1, 2, 3, 4, 5})

This is a reversible decision.

popematt · 2023-12-01T23:11:13Z

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

    }

    private data class ContainerInfo(
        var type: ContainerType? = null,
        var isDelimited: Boolean = false,
        var position: Long = -1,
-        var length: Long = -1,
+        var length: Long = 0,


🗺️ As I was working on annotations, I realized that it made more sense to always initialize the length of the ContainerInfo to 0. I also added a clear() function after I realized I might be leaking state between containers.

popematt · 2023-12-01T23:16:23Z

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

-
-        // Update the container length (unless it's Top)
-        if (currentContainer.type != Top) currentContainer.length += numBytesWritten
+        currentContainer.length += numBytesWritten


🗺️ A long is pretty big, and it doesn't hurt anyone to have the length of Top being tracked, so I got rid of the branching here (and similarly in stepOut()).

src/test/java/com/amazon/ion/impl/bin/IonRawBinaryWriterTest_1_1.kt

tgregg · 2023-12-05T01:23:39Z

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

     * [writeAnnotations] may be called more than once to build up a list of annotations.
     */
-    fun writeAnnotations(annotation0: Int, annotation1: Int, vararg annotations: Int)
+    fun writeAnnotations(annotations: IntArray)


Does this ever need to be called from Java? What does that look like?

tgregg · 2023-12-05T01:25:33Z

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

+     * are being added one by one.
+     */
+    private inline fun ensureAnnotationSpace(n: Int) {
+        if (annotationsIdBuffer.size < n || annotationsTextBuffer.size < n) {


Should these grow independently? Does that significantly complicate things?

The way I have it implemented right now, if position x of annotatationsTextBuffer is null, then we know to use the value from annotationsIdBuffer, so it does simplify the write logic if annotatationsTextBuffer is at least as large as annotationsIdBuffer, and if one has to be as least as large as the other, it simplifies the "growing" logic if we just make them the same size. I am not too concerned about the efficiency of growing both arrays at the same time because realistically, we don't expect there to be 10+ annotations for nearly all use cases.

tgregg · 2023-12-05T01:37:28Z

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

+        }
+        if (annotationFlexSymFlag == FLEX_SYMS_REQUIRED) {
+            buffer.writeByte(OpCodes.ANNOTATIONS_MANY_FLEX_SYM)
+            buffer.reserve(lengthPrefixPreallocation)


I wonder if this should be capped at 1 rather than using the same value used for containers. It seems less likely that your annotations would grow larger than can be described in a 1-byte FlexUInt.

That said, making a change here is probably only valuable if it simplifies the code.

It does marginally simplify the code because the minimum we always need to preallocate is 1, so if we're also capping it at 1, then we can just hard-code it to 1.

tgregg · 2023-12-07T21:01:09Z

src/main/java/com/amazon/ion/impl/bin/WriteBuffer.java

+            return writeFlexInt(sid);
+        } else {
+            writeByte((byte) 0x01);
+            writeByte((byte) 0x70);


What do you think about changing the spec so that this is 0x90 instead? That way it at least aligns with the upper nibble type (9 = symbol, whereas 7 = timestamp)

I like that, and I suspect that was probably the intention, but then parts of the spec were modified without fully updating other parts of the spec.

codecov · 2023-12-07T22:12:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (ion-11-encoding@79c8e5c). Click here to learn what that means.

Additional details and impacted files

@@                Coverage Diff                 @@
##             ion-11-encoding     #661   +/-   ##
==================================================
  Coverage                   ?   64.64%           
  Complexity                 ?     5703           
==================================================
  Files                      ?      199           
  Lines                      ?    24695           
  Branches                   ?     4386           
==================================================
  Hits                       ?    15963           
  Misses                     ?     7433           
  Partials                   ?     1299

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

popematt added 2 commits December 1, 2023 14:31

Improve factoring of IonRawBinaryWriterTest_1_1

bfcc2d4

Adds annotations to IonRawBinaryWriter_1_1

7d04b42

popematt commented Dec 1, 2023

View reviewed changes

popematt changed the title ~~Ion11writer annotations~~ IonRawBinaryWriter_1_1 annotations Dec 1, 2023

popematt requested a review from tgregg December 2, 2023 00:55

tgregg reviewed Dec 5, 2023

View reviewed changes

popematt mentioned this pull request Dec 7, 2023

Run CI builds for all branches #663

Merged

popematt added 2 commits December 7, 2023 08:32

Adds writeFlexSym to WriteBuffer

68180af

Adds suggested changes

cd575a8

tgregg approved these changes Dec 7, 2023

View reviewed changes

Adds suggested changes

de03f30

popematt merged commit 015c6a0 into amazon-ion:ion-11-encoding Dec 7, 2023
8 of 32 checks passed

popematt deleted the ion11writer-annotations branch December 7, 2023 22:18

tgregg pushed a commit that referenced this pull request Jan 22, 2024

Adds annotations to IonRawBinaryWriter_1_1 (#661)

af3183d

tgregg pushed a commit that referenced this pull request Feb 10, 2024

Adds annotations to IonRawBinaryWriter_1_1 (#661)

da9d4ce

tgregg pushed a commit that referenced this pull request May 2, 2024

Adds annotations to IonRawBinaryWriter_1_1 (#661)

733ccec

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

Adds annotations to IonRawBinaryWriter_1_1 (#661)

2ee478b

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

Adds annotations to IonRawBinaryWriter_1_1 (#661)

add35d8

tgregg pushed a commit that referenced this pull request Dec 13, 2024

Adds annotations to IonRawBinaryWriter_1_1 (#661)

1204458

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IonRawBinaryWriter_1_1 annotations #661

IonRawBinaryWriter_1_1 annotations #661

popematt commented Dec 1, 2023

popematt Dec 1, 2023

tgregg Dec 5, 2023

popematt Dec 6, 2023 •

edited

Loading

popematt Dec 1, 2023

popematt Dec 1, 2023

tgregg Dec 5, 2023

tgregg Dec 5, 2023

popematt Dec 6, 2023

tgregg Dec 5, 2023

popematt Dec 6, 2023

tgregg Dec 7, 2023

popematt Dec 7, 2023

codecov bot commented Dec 7, 2023

IonRawBinaryWriter_1_1 annotations #661

IonRawBinaryWriter_1_1 annotations #661

Conversation

popematt commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popematt Dec 6, 2023 • edited Loading

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

codecov bot commented Dec 7, 2023

Codecov Report

popematt Dec 6, 2023 •

edited

Loading