Includes various small fixes for tagless parameters, expression groups, and system macro invocations. #975

tgregg · 2024-10-16T20:45:55Z

Description of changes:

Includes various small fixes for tagless parameters, expression groups, and system macro invocations. See comments inline for a guide.

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

…s, and system macro invocations.

tgregg · 2024-10-16T20:46:52Z

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

@@ -3361,6 +3361,10 @@ private void setCheckpointAfterValueHeader() {
            case START_CONTAINER:
                setCheckpoint(CheckpointLocation.AFTER_CONTAINER_HEADER);
                break;
+            case NEEDS_INSTRUCTION:
+                // A macro invocation header has just been read.
+                setCheckpoint(CheckpointLocation.BEFORE_UNANNOTATED_TYPE_ID);


This moves the cursor past the macro invocation header, positioning it on the first argument (if any). This method is called when parsing expression groups.

tgregg · 2024-10-16T20:47:53Z

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

@@ -1449,7 +1449,7 @@ private void readGroupExpression(Macro.Parameter parameter, List<Expression.EExp
            int startIndex = expressions.size();
            expressions.add(Expression.Placeholder.INSTANCE);
            boolean isSingleton = true;
-            while (nextGroupedValue() != Event.NEEDS_INSTRUCTION) {
+            while (nextGroupedValue() != Event.NEEDS_INSTRUCTION || isMacroInvocation()) {


NEEDS_INSTRUCTION is used either when the end of the group has been reached, or when a macro invocation header has been encountered. When it's the latter, we need to read the invocation as an expression to continue progressing through the group.

tgregg · 2024-10-16T20:48:23Z

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

@@ -1560,7 +1560,7 @@ protected PresenceBitmap loadPresenceBitmapIfNecessary(List<Macro.Parameter> sig

        @Override
        protected boolean isMacroInvocation() {
-            return valueTid.isMacroInvocation;
+            return valueTid != null && valueTid.isMacroInvocation;


This method is now called when the cursor may be at the end of the group, at which point valueTid will be null.

tgregg · 2024-10-16T20:49:59Z

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

+            List<SymbolToken> annotations = getAnnotations();
+            boolean isSystemMacro = !annotations.isEmpty() && SystemSymbols_1_1.ION.getText().equals(annotations.get(0).getText());


The added macroInvocationInTaggedExpressionGroup test exercises this because it invokes the values macro from a context that does not export the Ion 1.1 system macros. The invocation looks like (:$ion::values ...)

tgregg · 2024-10-16T20:52:28Z

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

@@ -859,12 +859,12 @@ class IonRawBinaryWriter_1_1 internal constructor(
                    thisContainerTotalLength++
                } else if (isTagless) {
                    // End tagless group -- write the number of expressions, end with FlexUInt 0
-                    thisContainerTotalLength += writeCurrentContainerLength(lengthPrefixPreallocation)
+                    thisContainerTotalLength += writeCurrentContainerLength(maxOf(1, lengthPrefixPreallocation))


This is now symmetrical with stepInExpressionGroup for this case, which always reserves at least one byte for the length. Without this fix, the byte is reserved in the buffer and a patch point is added, resulting in an erroneous extra byte in the final output. The same can be said for the other change in this file.

This change is verified by the added taglessExpressionGroup test.

tgregg · 2024-10-16T20:52:49Z

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

                    buffer.writeByte(FlexInt.ZERO)
                    thisContainerTotalLength++
                } else if (currentContainer.isLengthPrefixed) {
                    // Length-prefixed, tagged -- write the number of bytes
-                    thisContainerTotalLength += writeCurrentContainerLength(lengthPrefixPreallocation)
+                    thisContainerTotalLength += writeCurrentContainerLength(maxOf(1, lengthPrefixPreallocation))


Exercised by the added macroInvocationInTaggedExpressionGroup test.

tgregg · 2024-10-16T20:53:31Z

src/main/java/com/amazon/ion/impl/macro/MacroCompiler.kt

+            val parameterEncoding = when (annotations.size) {
+                0 -> Macro.ParameterEncoding.Tagged
+                1 -> {
+                    val encodingText = annotations[0].text
+                    val encoding = Macro.ParameterEncoding.entries.singleOrNull { it.ionTextName == encodingText }
+                    if (encoding == null) {
+                        // TODO: Check for macro-shaped parameter encodings, and only if it's still null, we throw.
+                        throw IonException("unsupported parameter encoding $annotations")
+                    }
+                    encoding
+                }
+                2 -> TODO("Qualified references for macro-shaped parameters")
+                else -> throw IonException("unsupported parameter encoding $annotations")
+            }


This patch is contributed by @popematt, and is exercised by the added taglessExpressionGroup test.

tgregg · 2024-10-16T20:54:18Z

src/test/java/com/amazon/ion/impl/EncodingDirectiveCompilationTest.java

@@ -136,6 +141,30 @@ private static void writeMacroSignature(IonRawWriter_1_1 writer, Map<String, Int
        writer.stepOut();
    }

+    private static void writeMacroSignatureFromDatagram(IonRawWriter_1_1 writer, Map<String, Integer> symbols, IonDatagram... signature) {


This allows us to specify tagless encodings. The other variant just takes a String, without a way to add an encoding annotation.

Fascinating use of IonDatagram for "unmodeled sequence of values". I did a double-take.

popematt

Thanks!

jobarr-amzn · 2024-10-16T21:04:03Z

src/test/java/com/amazon/ion/impl/EncodingDirectiveCompilationTest.java

@@ -136,6 +141,30 @@ private static void writeMacroSignature(IonRawWriter_1_1 writer, Map<String, Int
        writer.stepOut();
    }

+    private static void writeMacroSignatureFromDatagram(IonRawWriter_1_1 writer, Map<String, Integer> symbols, IonDatagram... signature) {


Fascinating use of IonDatagram for "unmodeled sequence of values". I did a double-take.

jobarr-amzn · 2024-10-16T21:29:41Z

src/test/java/com/amazon/ion/impl/EncodingDirectiveCompilationTest.java

+            if (parameter.size() > 2) {
+                throw new IllegalStateException("Parameters can only have two components: a name and a cardinality.");
+            }


Suggested change

if (parameter.size() > 2) {

throw new IllegalStateException("Parameters can only have two components: a name and a cardinality.");

}

if (parameter.size() > 2) {

throw new IllegalStateException("Parameters can only have two components: a name and a cardinality.");

}

if (parameter.size() < 1) {

throw new IllegalStateException("Parameters must have at least one component: a name.");

}

jobarr-amzn · 2024-10-16T21:34:19Z

src/test/java/com/amazon/ion/impl/EncodingDirectiveCompilationTest.java

+        writer.stepInEExp(0, false, expectedMacro); {
+            writer.stepInExpressionGroup(true); {
+                writer.stepInEExp(SystemMacro.Values); {
+                    writer.writeString("bar");
+                } writer.stepOut();
+            } writer.stepOut();
+        } writer.stepOut();


I see you're also using the stepIn/Out idiom from old IonJava tests :D

Includes various small fixes for tagless parameters, expression group…

d9578c4

…s, and system macro invocations.

tgregg requested review from jobarr-amzn and popematt October 16, 2024 20:45

tgregg commented Oct 16, 2024

View reviewed changes

popematt approved these changes Oct 16, 2024

View reviewed changes

tgregg merged commit d402b46 into ion-11-encoding Oct 16, 2024
17 checks passed

tgregg deleted the ion-11-encoding-invocation-in-group branch October 16, 2024 21:36

jobarr-amzn approved these changes Oct 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Includes various small fixes for tagless parameters, expression groups, and system macro invocations. #975

Includes various small fixes for tagless parameters, expression groups, and system macro invocations. #975

tgregg commented Oct 16, 2024

tgregg Oct 16, 2024

tgregg Oct 16, 2024

tgregg Oct 16, 2024

tgregg Oct 16, 2024

tgregg Oct 16, 2024 •

edited

Loading

tgregg Oct 16, 2024

tgregg Oct 16, 2024

tgregg Oct 16, 2024

jobarr-amzn Oct 16, 2024

popematt left a comment

jobarr-amzn Oct 16, 2024

jobarr-amzn Oct 16, 2024

jobarr-amzn Oct 16, 2024

		List<SymbolToken> annotations = getAnnotations();
		boolean isSystemMacro = !annotations.isEmpty() && SystemSymbols_1_1.ION.getText().equals(annotations.get(0).getText());

Includes various small fixes for tagless parameters, expression groups, and system macro invocations. #975

Includes various small fixes for tagless parameters, expression groups, and system macro invocations. #975

Conversation

tgregg commented Oct 16, 2024

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

tgregg Oct 16, 2024 • edited Loading

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 left a 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

tgregg Oct 16, 2024 •

edited

Loading