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.

Sign up for GitHub

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

Jump to bottom

Add support for reading system e-expressions #961

Merged
merged 2 commits into from
Oct 8, 2024
Merged

Add support for reading system e-expressions #961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efe7a3a
Add partial support for reading system e-expressions
popematt Oct 4, 2024
d63262f
Adds suggested changes
popematt Oct 8, 2024
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
43 changes: 23 additions & 20 deletions src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.amazon.ion.impl.macro.MacroEvaluator;
import com.amazon.ion.impl.macro.MacroEvaluatorAsIonReader;
import com.amazon.ion.impl.macro.MacroRef;
import com.amazon.ion.impl.macro.SystemMacro;

import java.io.InputStream;
import java.math.BigDecimal;
Expand Down Expand Up @@ -112,11 +113,11 @@
// The symbol IDs for the annotations on the current value.
private final IntList annotationSids;

// The IonReader-like MacroEvaluator that this core reader delegates to when evaluating a macro invocation.
protected MacroEvaluatorAsIonReader macroEvaluatorIonReader = null;

// The core MacroEvaluator that this core reader delegates to when evaluating a macro invocation.
private MacroEvaluator macroEvaluator = null;
private final MacroEvaluator macroEvaluator = new MacroEvaluator();

// The IonReader-like MacroEvaluator that this core reader delegates to when evaluating a macro invocation.
protected MacroEvaluatorAsIonReader macroEvaluatorIonReader = new MacroEvaluatorAsIonReader(macroEvaluator);

// The encoding context (macro table) that is currently active.
private EncodingContext encodingContext = null;
Expand Down Expand Up @@ -1225,8 +1226,6 @@
*/
private void installMacros() {
encodingContext = new EncodingContext(newMacros);
macroEvaluator = new MacroEvaluator();
macroEvaluatorIonReader = new MacroEvaluatorAsIonReader(macroEvaluator);
}

/**
Expand Down Expand Up @@ -1529,9 +1528,6 @@
*/
private void readValueAsExpression(List<Expression.EExpressionBodyExpression> expressions) {
if (valueTid.isMacroInvocation) {
if (isSystemInvocation()) {
throw new UnsupportedOperationException("TODO: system macros");
}
collectEExpressionArgs(expressions); // TODO avoid recursion
return;
}
Expand Down Expand Up @@ -1664,17 +1660,24 @@
* @param expressions receives the expressions as they are materialized.
*/
private void collectEExpressionArgs(List<Expression.EExpressionBodyExpression> expressions) {
if (isSystemInvocation()) {
throw new UnsupportedOperationException("System macro invocations not yet supported.");
}
Macro macro;
long id = getMacroInvocationId();
if (id > Integer.MAX_VALUE) {
throw new IonException("Macro addresses larger than 2147483647 are not supported by this implementation.");
}
MacroRef address = MacroRef.byId((int) id);
Macro macro = encodingContext.getMacroTable().get(address);
if (macro == null) {
throw new IonException(String.format("Encountered an unknown macro address: %d.", id));
if (isSystemInvocation()) {
macro = SystemMacro.get((int) id);
if (macro == null) {
throw new UnsupportedOperationException("System macro " + id + " not yet supported.");

Check warning on line 1668 in src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java#L1668 

Added line #L1668 was not covered by tests
}
} else {
if (id > Integer.MAX_VALUE) {
throw new IonException("Macro addresses larger than 2147483647 are not supported by this implementation.");

Check warning on line 1672 in src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java#L1672 

Added line #L1672 was not covered by tests
}
MacroRef address = MacroRef.byId((int) id);
Map<MacroRef, Macro> macroTable = encodingContext.getMacroTable();
macro = macroTable.get(address);

if (macro == null) {
throw new IonException(String.format("Encountered an unknown macro address: %d.", id));

Check warning on line 1679 in src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/com/amazon/ion/impl/IonReaderContinuableCoreBinary.java#L1679 

Added line #L1679 was not covered by tests
}
}
PresenceBitmap presenceBitmap = new PresenceBitmap();
List<Macro.Parameter> signature = macro.getSignature();
Expand Down Expand Up @@ -1768,7 +1771,7 @@
event = super.nextValue();
}
if (valueTid != null && valueTid.isMacroInvocation) {
if (macroEvaluator == null) {
if (encodingContext == null && !isSystemInvocation()) {
// If the macro evaluator is null, it means there is no active macro table. Do not attempt evaluation,
// but allow the user to do a raw read of the parameters if this is a core-level reader.
// TODO this is used in the tests for the core binary reader. If it cannot be useful elsewhere, remove
Expand Down
51 changes: 51 additions & 0 deletions src/test/java/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5934,5 +5934,56 @@ public void prefixedAnnotatedContainerInsideDelimitedAnnotatedContainerPreserves
closeAndCount();
}


@ParameterizedTest(name = "[{index}] {0}")
@CsvSource({
// (:values 0) (:values 1)
"EF 01 01 60 EF 01 01 61 01",
// (:values (: values 0)) 1
"EF 01 01 EF 01 01 60 61 01",
// (:values) 0 (:values) 1
"EF 01 00 60 EF 01 00 61 01",
// TODO: Determine why the state isn't being properly set after invoking a macro that produces nothing
// See https://github.com/amazon-ion/ion-java/issues/962
// (:values) (:values 0) 1
// "EF 01 00 EF 01 01 60 61 01",
})
public void invokeValuesUsingSystemMacroOpcode(String bytes) throws Exception {
invokeValuesUsingSystemMacroOpcodeHelper(true, bytes);
invokeValuesUsingSystemMacroOpcodeHelper(false, bytes);
}

@ParameterizedTest(name = "[{index}] {0}")
@CsvSource({
// (:values (:: ) ) 0 1
"EF 01 02 01 F0 60 61 01",
// (:values 0 1) // using length-prefixed expression group
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// (:values 0 1) // using length-prefixed expression group
// (:values (:: 0 1)) // using length-prefixed expression group

Is this right?

Copy link
Contributor Author

@popematt popematt Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Either one is legal since we can use the implicit rest param for the argument to values. I've only used (:: ) in some places to make it clear that the encoding is an empty expression group or a single-arg expression group.

If you have a preference for always being explicit about expression groups in the tests here, I can change it.

"EF 01 02 07 60 61 01",
// (:values 0 1) // using delimited expression group
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// (:values 0 1) // using delimited expression group
// (:values (:: 0 1)) // using delimited expression group

Is this right?

"EF 01 02 01 60 61 01 F0",
// (:values (:: 0) (:values (:: 1))
"EF 01 02 03 60 EF 01 02 05 61 01",
// (:values (:values 0 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// (:values (:values 0 1))
// (:values (:values 0 1))

I guess we could choose not to use the expression group syntax for all of these, even though that's how they're encoded in binary...

"EF 01 01 EF 01 02 07 60 61 01",
})
public void invokeValuesWithExpressionGroupsUsingSystemMacroOpcode(String bytes) throws Exception {
invokeValuesUsingSystemMacroOpcodeHelper(true, bytes);
// TODO: Determine why the checkpoint in the cursor isn't being set correctly before calling slowFillValue()
// specifically while reading from a length-prefixed expression group
// See https://github.com/amazon-ion/ion-java/issues/963
// invokeValuesUsingSystemMacroOpcodeHelper(false, bytes);
}

private void invokeValuesUsingSystemMacroOpcodeHelper(boolean constructFromBytes, String bytes) throws Exception {
byte[] input = withIvm(1, hexStringToByteArray(cleanCommentedHexBytes(bytes)));
readerBuilder = readerBuilder.withIncrementalReadingEnabled(false);
reader = readerFor(readerBuilder, constructFromBytes, input);
assertSequence(
next(IonType.INT), intValue(0),
next(IonType.INT), intValue(1),
next(null)
);
}

// TODO Ion 1.1 symbol tables with all kinds of annotation encodings (opcodes E4 - E9, inline and SID)
}
Loading