-
Notifications
You must be signed in to change notification settings - Fork 110
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
Adds support for Ion 1.1 inline symbols to the system-level binary reader. #860
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,5 @@ | ||
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package com.amazon.ion.impl; | ||
|
||
import com.amazon.ion.Decimal; | ||
|
@@ -19,8 +18,10 @@ | |
import java.math.BigDecimal; | ||
import java.math.BigInteger; | ||
import java.util.ArrayDeque; | ||
import java.util.ArrayList; | ||
import java.util.Date; | ||
import java.util.Iterator; | ||
import java.util.List; | ||
import java.util.Queue; | ||
|
||
import static com.amazon.ion.IonCursor.Event.NEEDS_DATA; | ||
|
@@ -46,8 +47,15 @@ final class IonReaderNonContinuableSystem implements IonReader { | |
*/ | ||
IonReaderNonContinuableSystem(IonReaderContinuableCore reader) { | ||
this.reader = reader; | ||
reader.registerIvmNotificationConsumer((x, y) -> { | ||
pendingIvmSids.add(SystemSymbols.ION_1_0_SID); // TODO generalize for Ion 1.1 | ||
reader.registerIvmNotificationConsumer((major, minor) -> { | ||
if (major != 1 || minor > 1) { | ||
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? | ||
} | ||
Comment on lines
+54
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I have some ideas, but I haven't checked them for feasibility w.r.t. backwards compatibility with existing public APIs. We don't actually need the SID of the IVM. We just need some value that represents which version it is. I don't think we should re-use Alternately, maybe we could say that the system reader doesn't give you IVMs as symbols—it requires registering an IVM listener just like the regular reader. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we're on the same page here. I would like if we could avoid requiring the Ion 1.1 writer to recognize SIDs as IVMs, while still allowing IVMs to be propagated from the system reader to a user-level writer. We may have to add some extra logic / callback registration between the writer and system reader to make that work, but it should be doable. |
||
}); | ||
} | ||
|
||
|
@@ -224,10 +232,14 @@ public String stringValue() { | |
prepareScalar(); | ||
String value; | ||
if (type == IonType.SYMBOL) { | ||
int sid = reader.symbolValueId(); | ||
value = getSymbolTable().findKnownSymbol(sid); | ||
if (value == null) { | ||
throw new UnknownSymbolException(sid); | ||
if (reader.hasSymbolText()) { | ||
value = reader.getSymbolText(); | ||
} else { | ||
int sid = reader.symbolValueId(); | ||
value = getSymbolTable().findKnownSymbol(sid); | ||
if (value == null) { | ||
throw new UnknownSymbolException(sid); | ||
} | ||
} | ||
} else { | ||
value = reader.stringValue(); | ||
|
@@ -269,33 +281,43 @@ public String[] getTypeAnnotations() { | |
if (pendingIvmSid != -1 || !reader.hasAnnotations()) { | ||
return _Private_Utils.EMPTY_STRING_ARRAY; | ||
} | ||
int[] annotationIds = reader.getAnnotationIds(); | ||
String[] annotations = new String[annotationIds.length]; | ||
// Note: it is not expected that the system reader is used in performance-sensitive applications; hence, | ||
// no effort is made optimize the following. | ||
List<String> annotations = new ArrayList<>(); | ||
SymbolTable symbolTable = getSymbolTable(); | ||
for (int i = 0; i < annotationIds.length; i++) { | ||
int sid = annotationIds[i]; | ||
String annotation = symbolTable.findKnownSymbol(sid); | ||
if (annotation == null) { | ||
throw new UnknownSymbolException(sid); | ||
reader.consumeAnnotationTokens((token) -> { | ||
String text = token.getText(); | ||
if (text == null) { | ||
int sid = token.getSid(); | ||
text = symbolTable.findKnownSymbol(sid); | ||
if (text == null) { | ||
throw new UnknownSymbolException(sid); | ||
} | ||
} | ||
annotations[i] = annotation; | ||
} | ||
return annotations; | ||
annotations.add(text); | ||
}); | ||
return annotations.toArray(_Private_Utils.EMPTY_STRING_ARRAY); | ||
} | ||
|
||
@Override | ||
public SymbolToken[] getTypeAnnotationSymbols() { | ||
if (pendingIvmSid != -1 || !reader.hasAnnotations()) { | ||
return SymbolToken.EMPTY_ARRAY; | ||
} | ||
int[] annotationIds = reader.getAnnotationIds(); | ||
SymbolToken[] annotationSymbolTokens = new SymbolToken[annotationIds.length]; | ||
// Note: it is not expected that the system reader is used in performance-sensitive applications; hence, | ||
// no effort is made optimize the following. | ||
List<SymbolToken> annotations = new ArrayList<>(); | ||
SymbolTable symbolTable = getSymbolTable(); | ||
for (int i = 0; i < annotationIds.length; i++) { | ||
int sid = annotationIds[i]; | ||
annotationSymbolTokens[i] = new SymbolTokenImpl(symbolTable.findKnownSymbol(sid), sid); | ||
} | ||
return annotationSymbolTokens; | ||
reader.consumeAnnotationTokens((token) -> { | ||
String text = token.getText(); | ||
if (text != null) { | ||
annotations.add(token); | ||
} else { | ||
int sid = token.getSid(); | ||
annotations.add(new SymbolTokenImpl(symbolTable.findKnownSymbol(sid), sid)); | ||
} | ||
}); | ||
return annotations.toArray(SymbolToken.EMPTY_ARRAY); | ||
} | ||
|
||
@Override | ||
|
@@ -313,6 +335,9 @@ public int getFieldId() { | |
|
||
@Override | ||
public String getFieldName() { | ||
if (reader.hasFieldText()) { | ||
return reader.getFieldText(); | ||
} | ||
int sid = reader.getFieldId(); | ||
if (sid < 0) { | ||
return null; | ||
|
@@ -326,23 +351,37 @@ public String getFieldName() { | |
|
||
@Override | ||
public SymbolToken getFieldNameSymbol() { | ||
int sid = reader.getFieldId(); | ||
if (sid < 0) { | ||
return null; | ||
String fieldText; | ||
int sid = -1; | ||
if (reader.hasFieldText()) { | ||
fieldText = reader.getFieldText(); | ||
} else { | ||
sid = reader.getFieldId(); | ||
if (sid < 0) { | ||
return null; | ||
} | ||
fieldText = getSymbolTable().findKnownSymbol(sid); | ||
} | ||
return new SymbolTokenImpl(getSymbolTable().findKnownSymbol(sid), sid); | ||
return new SymbolTokenImpl(fieldText, sid); | ||
} | ||
|
||
@Override | ||
public SymbolToken symbolValue() { | ||
int sid; | ||
String symbolText; | ||
int sid = -1; | ||
if (pendingIvmSid != -1) { | ||
sid = pendingIvmSid; | ||
symbolText = getSymbolTable().findKnownSymbol(sid); | ||
} else { | ||
prepareScalar(); | ||
sid = reader.symbolValueId(); | ||
if (reader.hasSymbolText()) { | ||
symbolText = reader.getSymbolText(); | ||
} else { | ||
sid = reader.symbolValueId(); | ||
symbolText = getSymbolTable().findKnownSymbol(sid); | ||
} | ||
} | ||
return new SymbolTokenImpl(getSymbolTable().findKnownSymbol(sid), sid); | ||
return new SymbolTokenImpl(symbolText, sid); | ||
} | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it easier to implement this way, or can we just say that the consumer will receive no values if there are no annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an internal API, so I'm happy to have a stricter contract that cuts down on extra branching.