Implements writeValues for IonManagedWriter_1_1 #867

popematt · 2024-05-24T00:01:02Z

Issue #, if available:

N/A

Description of changes:

Updates existing readers to handle Ion 1.1 IVM
Updates existing readers to detect local symbol tables whether they are annotated with $3 or $ion_symbol_table.
Fixes the check for user-supplied symbol tables in IonManagedWriter_1_1
Implements writeValues for IonManagedWriter_1_1
Completely refactored the Ion 1.1 round trip tests
Added some initial test cases in IonManagedWriter_1_1_Test
Fixed the user-supplied symbol table detection logic in IonManagedWriter_1_1

Issues discovered in the process that are not fixed:

IonReaderTextSystemX.getFieldName does not fulfill the contract for IonReader. It throws UnknownSymbolException rather than returning null for getFieldName
There's an IllegalStateException being thrown when reading a symbol table annotated with an inline text annotation in IonReaderContinuableApplicationBinary. Reproducible with Ion_1_1_RoundTripTest.
IonReaderContinuableApplicationBinary.getSymbolToken() throws UnknownSymbolException if the symbol has unknown text in some cases. Reproducible with Ion_1_1_RoundTripTest.
IonReaderNonContinuableSystem throws an exception for some cases when it can't find any user values. Reproducible with Ion_1_1_RoundTripTest.
_Private_IonSystem.systemIterate seems to do the same thing as IonSystem.iterate, which seems fishy.
Sharp edge—it would be nice if there was some interface that all IonWriterBuilders implemented with even just the build(OutputStream) method. Instead, I've had to use (OutputStream) -> IonWriter to wrap the builders for Ion_1_1_RoundTripTest.

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

codecov · 2024-05-24T00:09:08Z

Codecov Report

Attention: Patch coverage is 75.00000% with 37 lines in your changes are missing coverage. Please review.

Please upload report for BASE (ion-11-encoding@4167fa3). Learn more about missing BASE report.

Files	Patch %	Lines
...va/com/amazon/ion/impl/bin/IonManagedWriter_1_1.kt	72.09%	20 Missing and 4 partials ⚠️
...amazon/ion/impl/IonReaderNonContinuableSystem.java	80.00%	1 Missing and 5 partials ⚠️
...on/impl/IonReaderContinuableApplicationBinary.java	69.23%	2 Missing and 2 partials ⚠️
...n/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt	83.33%	0 Missing and 1 partial ⚠️
...n/java/com/amazon/ion/impl/IonReaderTreeUserX.java	83.33%	0 Missing and 1 partial ⚠️
.../com/amazon/ion/impl/bin/IonRawBinaryWriter_1_1.kt	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##             ion-11-encoding     #867   +/-   ##
==================================================
  Coverage                   ?   68.91%           
  Complexity                 ?     6289           
==================================================
  Files                      ?      182           
  Lines                      ?    25230           
  Branches                   ?     4504           
==================================================
  Hits                       ?    17388           
  Misses                     ?     6473           
  Partials                   ?     1369

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

popematt · 2024-05-24T05:12:47Z

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

+                if (marker.endIndex - marker.startIndex != ION_SYMBOL_TABLE_UTF8.length) return false;
+                int start = (int) marker.startIndex;
+                boolean isIonSymbolTable = true;
+                for (int i = 0; i < ION_SYMBOL_TABLE_UTF8.length; i++) {
+                    isIonSymbolTable &= buffer[start + i] == ION_SYMBOL_TABLE_UTF8[i];
+                }
+                return isIonSymbolTable;


🗺️ This bit of code shows up as untested in the codecov report because after this part succeeds, there can end up being an IllegalStateException (see L890), so the tests that cover this are currently disabled (see Ion_1_1_RoundTripTests.kt).

Is this a necessary part of the PR, even in its broken state, or should we just leave it out and fully address it in response to #866?

I think it's probably fine to leave it out. How is this instead?

Suggested change

if (marker.endIndex - marker.startIndex != ION_SYMBOL_TABLE_UTF8.length) return false;

int start = (int) marker.startIndex;

boolean isIonSymbolTable = true;

for (int i = 0; i < ION_SYMBOL_TABLE_UTF8.length; i++) {

isIonSymbolTable &= buffer[start + i] == ION_SYMBOL_TABLE_UTF8[i];

}

return isIonSymbolTable;

throw new UnsupportedOperationException("https://github.com/amazon-ion/ion-java/issues/866");

As it turns out, some of this code was used (How did I forget that? 🤦‍♂️) so I've reverted the change I made to remove it.

popematt · 2024-05-24T05:58:55Z

src/test/java/com/amazon/ion/Ion_1_1_RoundTripTest.kt

+
+        override val writerFn: (OutputStream) -> IonWriter = builder::build
+
+        @Disabled("Ion binary reader can't seem to discover symbol tables with inline annotations")


🗺️ This test reproduces one of the bugs mentioned in the PR summary.

popematt · 2024-05-24T06:04:39Z

src/test/java/com/amazon/ion/Ion_1_1_RoundTripTest.kt

+        val LOCAL_SKIP_LIST = setOf(
+            // Has an unknown, imported symbol
+            "symbolTablesUnknownText.ion",
+            // Skipped because there are no user values in these, and IonReaderNonContinuableSystem will throw an exception.


🗺️ These files can reproduce another one of the issues mentioned in the PR summary.

tgregg · 2024-05-24T23:53:11Z

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

+                if (marker.endIndex - marker.startIndex != ION_SYMBOL_TABLE_UTF8.length) return false;
+                int start = (int) marker.startIndex;
+                boolean isIonSymbolTable = true;
+                for (int i = 0; i < ION_SYMBOL_TABLE_UTF8.length; i++) {
+                    isIonSymbolTable &= buffer[start + i] == ION_SYMBOL_TABLE_UTF8[i];
+                }
+                return isIonSymbolTable;


Is this a necessary part of the PR, even in its broken state, or should we just leave it out and fully address it in response to #866?

tgregg · 2024-05-25T00:06:35Z

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

+            finish()
+            systemData.writeIVM()
+        } else {
+            writeSymbol("\$ion_1_1")


This isn't an IVM... should this be an error? Or are we just saying that it's not the caller's responsibility know whether a real IVM is legal in this position?

I thought it should be an error (which is why I originally implemented it the way I did), but then I read the Javadoc for the inherited method.

Write an Ion version marker symbol to the output. This is the $ion_1_0 value currently (in later versions the number may change). In text output this appears as the text symbol. In binary this will be the symbol id if the writer is in a list, sexp or struct. If the writer is currently at the top level this will write the "magic cookie" value. Writing a version marker will reset the symbol table to be the system symbol table.

This seems pretty clear that it should just write a symbol if the reader is in any container.

This reverts commit 09c16d7.

Implements writeValues for IonManagedWriter_1_1

dd39820

popematt force-pushed the write-values branch from a956e57 to dd39820 Compare May 24, 2024 00:03

popematt added 3 commits May 23, 2024 22:08

Adds unit tests for _private_hasFirstAnnotation()

c9db9b1

Clean up PendingIVM enum

bb4c3f9

More cleanup

859df80

popematt force-pushed the write-values branch from 0768513 to 859df80 Compare May 24, 2024 06:02

popematt commented May 24, 2024

View reviewed changes

popematt marked this pull request as ready for review May 24, 2024 06:14

popematt requested a review from tgregg May 24, 2024 06:14

tgregg approved these changes May 25, 2024

View reviewed changes

popematt and others added 3 commits May 24, 2024 17:44

Update IonReaderContinuableApplicationBinary.java

09c16d7

Revert "Update IonReaderContinuableApplicationBinary.java"

1751295

This reverts commit 09c16d7.

Adds another Ion 1.1 rountrip test case

32fcf4d

popematt merged commit 2a09b71 into amazon-ion:ion-11-encoding May 25, 2024
19 of 36 checks passed

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

Implements writeValues for IonManagedWriter_1_1 (#867)

3461f7f

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

Implements writeValues for IonManagedWriter_1_1 (#867)

72c58b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements writeValues for IonManagedWriter_1_1 #867

Implements writeValues for IonManagedWriter_1_1 #867

popematt commented May 24, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading

popematt May 24, 2024

tgregg May 24, 2024

popematt May 25, 2024

popematt May 25, 2024

popematt May 24, 2024

popematt May 24, 2024

tgregg May 24, 2024

tgregg May 25, 2024

popematt May 25, 2024


		override val writerFn: (OutputStream) -> IonWriter = builder::build

		@Disabled("Ion binary reader can't seem to discover symbol tables with inline annotations")

Implements writeValues for IonManagedWriter_1_1 #867

Implements writeValues for IonManagedWriter_1_1 #867

Conversation

popematt commented May 24, 2024 • edited Loading

codecov bot commented May 24, 2024 • edited Loading

Codecov Report

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

popematt commented May 24, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading