Adds read support for Ion 1.1 system symbols #944
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ion-11-encoding #944 +/- ##
==================================================
Coverage ? 70.05%
Complexity ? 6870
==================================================
Files ? 196
Lines ? 27070
Branches ? 4899
==================================================
Hits ? 18964
Misses ? 6593
Partials ? 1513 ☔ View full report in Codecov by Sentry. |
| // TODO: We could pretend $0 is a system symbol and consolidate some of the branches here. Is it worth it? | ||
| if (nextByte == FLEX_SYM_SYSTEM_SYMBOL_OFFSET) { |
There was a problem hiding this comment.
I'd be fine with that, if you can get it to work. However, we return 0 from this branch and -1 from the system symbol branch, so there may need to be the same amount of branching either way.
src/main/java/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java
Outdated
Show resolved
Hide resolved
| // FIXME: This should take into account the version at the point in the stream. | ||
| resetImports(1, 0); |
There was a problem hiding this comment.
What's stopping us from passing in the real major and minor version here?
There was a problem hiding this comment.
With the current implementation, the version of the symbol table is determined by the system symbol table that it imports. In Ion 1.1, the local symbol table does not necessarily import any system table. I think there's a lot more work that will need to be done in order to get Ion 1.1 and the current SymbolTable API to work together.
| } else { | ||
| id = readFixedUInt_1_1(valueMarker.startIndex, valueMarker.endIndex); | ||
|
|
||
| // FIXME: This is a hack that works as long as our system symbol table doesn't grow to |
There was a problem hiding this comment.
I'm fine tracking this in an issue. This should work for the foreseeable future.
There was a problem hiding this comment.
I will create an issue with relevant perma-links to the code once this PR is merged.
src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java
Outdated
Show resolved
Hide resolved
Issue #, if available:
None.
Description of changes:
EEop code.A0for "unknown symbol" to60and90for empty symbol to75.@Disabledannotation from disabled tests inIonManagedWriter_1_1_TestIonTypeID. (I.e. wasSYMBOL(1), now0xEE(SYMBOL,1)) That was useful to me while debugging things.EncodingDirectiveCompilationTest. For some reason, I couldn't seem to understand how these tests were supposed to work in the first place, so I was unable to update them to get them to work with my changes. Because almost all of the other tests are working, I am fairly confident that the problem lies in the tests rather than in the changes I've made. I will need to figure this out and update, replace, or remove this test class, but I didn't want to block this PR on it.FIXME:note about it in the code. This works for now... but should probably be fixed.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.