Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implements writeValues for IonManagedWriter_1_1 #867

Merged
merged 7 commits into from
May 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/main/java/com/amazon/ion/impl/IonRawTextWriter_1_1.kt
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,17 @@ class IonRawTextWriter_1_1 internal constructor(
numAnnotations = 0
}

override fun _private_hasFirstAnnotation(sid: Int, text: String?): Boolean {
if (numAnnotations == 0) return false
if (sid >= 0 && annotationsIdBuffer[0] == sid) {
return true
}
if (text != null && annotationsTextBuffer[0] == text) {
return true
}
return false
}

override fun _private_hasFieldName(): Boolean = hasFieldName

override fun writeFieldName(sid: Int) {
Expand Down
6 changes: 6 additions & 0 deletions src/main/java/com/amazon/ion/impl/IonRawWriter_1_1.kt
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,12 @@ interface IonRawWriter_1_1 {
*/
fun _private_clearAnnotations()

/**
* Returns true if the reader has at least one annotation set and the first annotation matches the
* given sid OR text.
*/
fun _private_hasFirstAnnotation(sid: Int, text: String?): Boolean

/**
* Writes one annotation for the next value.
* [writeAnnotations] may be called more than once to build up a list of annotations.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,7 @@
}
readImportMaxId();
break;
default: throw new IllegalStateException();
default: throw new IllegalStateException(state.toString());

Check warning on line 890 in src/main/java/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L890 was not covered by tests
}
}
}
Expand Down Expand Up @@ -920,14 +920,28 @@
* false.
*/
boolean startsWithIonSymbolTable() {
if (minorVersion == 0 || annotationTokenMarkers.isEmpty()) {
if (minorVersion == 0 && annotationSequenceMarker.startIndex >= 0) {
long savedPeekIndex = peekIndex;
peekIndex = annotationSequenceMarker.startIndex;
int sid = readVarUInt_1_0();
peekIndex = savedPeekIndex;
return ION_SYMBOL_TABLE_SID == sid;
}
return ION_SYMBOL_TABLE_SID == annotationTokenMarkers.get(0).endIndex;
if (minorVersion == 1) {
Marker marker = annotationTokenMarkers.get(0);
if (marker.startIndex < 0) {
return marker.endIndex == ION_SYMBOL_TABLE_SID;
} else {
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;
Comment on lines +935 to +941
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ 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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
return false;

Check warning on line 944 in src/main/java/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java

View check run for this annotation

Codecov / codecov/patch

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

Added line #L944 was not covered by tests
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,34 @@
*/
final class IonReaderNonContinuableSystem implements IonReader {

private static final SymbolToken IVM_1_0 = new SymbolTokenImpl(SystemSymbols.ION_1_0, SystemSymbols.ION_1_0_SID);
private static final SymbolToken IVM_1_1 = new SymbolTokenImpl("$ion_1_1", -1);

/**
* Represents an IVM that was read that has not yet been exposed as a Symbol value.
*/
private enum PendingIvm {
ION_1_0(IVM_1_0),
ION_1_1(IVM_1_1);

private final SymbolToken token;
PendingIvm(SymbolToken symbolToken) {
token = symbolToken;
}

static PendingIvm pendingIvmForVersionOrNull(int major, int minor) {
if (major != 1) return null;
if (minor == 0) return ION_1_0;
if (minor == 1) return ION_1_1;
return null;

Check warning on line 57 in src/main/java/com/amazon/ion/impl/IonReaderNonContinuableSystem.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/ion/impl/IonReaderNonContinuableSystem.java#L57

Added line #L57 was not covered by tests
}
}

private final IonReaderContinuableCore reader;
private IonType type = null;
private IonType typeAfterIvm = null;
private final Queue<Integer> pendingIvmSids = new ArrayDeque<>(1);
private int pendingIvmSid = -1;
private final Queue<PendingIvm> pendingIvms = new ArrayDeque<>(1);
private PendingIvm pendingIvm = null;

/**
* Constructs a new non-continuable system-level reader over the given continuable reader.
Expand All @@ -48,14 +71,11 @@
IonReaderNonContinuableSystem(IonReaderContinuableCore reader) {
this.reader = reader;
reader.registerIvmNotificationConsumer((major, minor) -> {
if (major != 1 || minor > 1) {
PendingIvm ivm = PendingIvm.pendingIvmForVersionOrNull(major, minor);
if (ivm == null) {
throw new IllegalStateException("The parser should have already thrown upon encountering this illegal IVM.");
}
if (minor == 0) {
pendingIvmSids.add(SystemSymbols.ION_1_0_SID);
} else {
// TODO how should this be handled for Ion 1.1?
}
pendingIvms.add(ivm);
});
}

Expand All @@ -70,20 +90,20 @@
* @return true if a value is ready to be presented to the user; otherwise, false.
*/
private boolean handleIvm() {
Integer ivmSid = pendingIvmSids.poll();
if (ivmSid != null) {
PendingIvm nextPendingIvm = pendingIvms.poll();
if (nextPendingIvm != null) {
// An IVM has been found between values.
if (typeAfterIvm == null) {
// Only save the type of the next user value the first time an IVM is encountered before that value.
typeAfterIvm = type;
}
// For consistency with the legacy implementation, the system reader surfaces IVMs as symbol values.
type = IonType.SYMBOL;
pendingIvmSid = ivmSid;
pendingIvm = nextPendingIvm;
return true;
} else if (pendingIvmSid != -1) {
} else if (pendingIvm != null) {
// All preceding IVMs have been surfaced. Restore the value that follows.
pendingIvmSid = -1;
pendingIvm = null;
type = typeAfterIvm;
typeAfterIvm = null;
return true;
Expand Down Expand Up @@ -162,7 +182,7 @@

@Override
public boolean isNullValue() {
return pendingIvmSid == -1 && reader.isNullValue();
return pendingIvm == null && reader.isNullValue();
}

@Override
Expand Down Expand Up @@ -226,8 +246,8 @@

@Override
public String stringValue() {
if (pendingIvmSid != -1) {
return getSymbolTable().findKnownSymbol(pendingIvmSid);
if (pendingIvm != null) {
return pendingIvm.token.getText();
}
prepareScalar();
String value;
Expand Down Expand Up @@ -278,7 +298,7 @@

@Override
public String[] getTypeAnnotations() {
if (pendingIvmSid != -1 || !reader.hasAnnotations()) {
if (pendingIvm != null || !reader.hasAnnotations()) {
return _Private_Utils.EMPTY_STRING_ARRAY;
}
// Note: it is not expected that the system reader is used in performance-sensitive applications; hence,
Expand All @@ -301,7 +321,7 @@

@Override
public SymbolToken[] getTypeAnnotationSymbols() {
if (pendingIvmSid != -1 || !reader.hasAnnotations()) {
if (pendingIvm != null || !reader.hasAnnotations()) {
return SymbolToken.EMPTY_ARRAY;
}
// Note: it is not expected that the system reader is used in performance-sensitive applications; hence,
Expand All @@ -322,7 +342,7 @@

@Override
public Iterator<String> iterateTypeAnnotations() {
if (pendingIvmSid != -1 || !reader.hasAnnotations()) {
if (pendingIvm != null || !reader.hasAnnotations()) {
return _Private_Utils.emptyIterator();
}
return _Private_Utils.stringIterator(getTypeAnnotations());
Expand Down Expand Up @@ -369,9 +389,8 @@
public SymbolToken symbolValue() {
String symbolText;
int sid = -1;
if (pendingIvmSid != -1) {
sid = pendingIvmSid;
symbolText = getSymbolTable().findKnownSymbol(sid);
if (pendingIvm != null) {
return pendingIvm.token;
} else {
prepareScalar();
if (reader.hasSymbolText()) {
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/com/amazon/ion/impl/IonReaderTextUserX.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import static com.amazon.ion.SystemSymbols.ION_1_0;
import static com.amazon.ion.SystemSymbols.ION_SYMBOL_TABLE;
import static com.amazon.ion.SystemSymbols.ION_SYMBOL_TABLE_SID;

import com.amazon.ion.IonCatalog;
import com.amazon.ion.IonType;
Expand Down Expand Up @@ -118,7 +119,7 @@ private final boolean has_next_user_value()
if (_value_type != null && !isNullValue() && IonType.DATAGRAM.equals(getContainerType())) {
switch (_value_type) {
case STRUCT:
if (_annotation_count > 0 && ION_SYMBOL_TABLE.equals(_annotations[0].getText())) {
if (_annotation_count > 0 && (ION_SYMBOL_TABLE.equals(_annotations[0].getText()) || ION_SYMBOL_TABLE_SID == _annotations[0].getSid())) {
_symbols = _lstFactory.newLocalSymtab(_catalog,
this,
true);
Expand Down Expand Up @@ -158,7 +159,7 @@ private final boolean has_next_user_value()
return (!_eof);
}

private static boolean isIonVersionMarker(String text)
static boolean isIonVersionMarker(String text)
{
return text != null && ION_VERSION_MARKER_REGEX.matcher(text).matches();
}
Expand Down
34 changes: 16 additions & 18 deletions src/main/java/com/amazon/ion/impl/IonReaderTreeUserX.java
Original file line number Diff line number Diff line change
@@ -1,23 +1,12 @@
/*
* Copyright 2007-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License").
* You may not use this file except in compliance with the License.
* A copy of the License is located at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* or in the "license" file accompanying this file. This file is distributed
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
* express or implied. See the License for the specific language governing
* permissions and limitations under the License.
*/

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0
package com.amazon.ion.impl;

import static com.amazon.ion.SymbolTable.UNKNOWN_SYMBOL_ID;
import static com.amazon.ion.SystemSymbols.ION_1_0_SID;
import static com.amazon.ion.SystemSymbols.ION_SYMBOL_TABLE;
import static com.amazon.ion.SystemSymbols.ION_SYMBOL_TABLE_SID;
import static com.amazon.ion.impl.IonReaderTextUserX.isIonVersionMarker;

import com.amazon.ion.IonCatalog;
import com.amazon.ion.IonDatagram;
Expand All @@ -29,6 +18,7 @@
import com.amazon.ion.Span;
import com.amazon.ion.SpanProvider;
import com.amazon.ion.SymbolTable;
import com.amazon.ion.SymbolToken;


final class IonReaderTreeUserX
Expand Down Expand Up @@ -111,9 +101,9 @@ boolean next_helper_user()
sid = _system_symtab.findSymbol(name);
}
}
if (sid == ION_1_0_SID
if ((sid == ION_1_0_SID || isIonVersionMarker(sym.symbolValue().getText()))
&& _next.getTypeAnnotationSymbols().length == 0) {
// $ion_1_0 is read as an IVM only if it is not annotated
// $ion_1_0 and other version markers are read as an IVM only if unannotated
SymbolTable symbols = _system_symtab;
_symbols = symbols;
push_symbol_table(symbols);
Expand All @@ -122,7 +112,7 @@ boolean next_helper_user()
}
}
else if (IonType.STRUCT.equals(next_type)
&& _next.findTypeAnnotation(ION_SYMBOL_TABLE) == 0
&& _next_has_ion_symbol_table_annotation()
) {
assert(_next instanceof IonStruct);
// read a local symbol table
Expand All @@ -143,6 +133,14 @@ else if (IonType.STRUCT.equals(next_type)
}
return (next_type != null);
}

private boolean _next_has_ion_symbol_table_annotation() {
SymbolToken[] annotations = _next.getTypeAnnotationSymbols();
if (annotations.length == 0) return false;
return annotations[0].getSid() == ION_SYMBOL_TABLE_SID
|| annotations[0].getText() == ION_SYMBOL_TABLE;
}

//
// This code handles the skipped symbol table
// support - it is cloned in IonReaderTextUserX
Expand Down
Loading
Loading