Macro implementations and conformance test fixes #1003

popematt · 2024-11-26T23:56:43Z

Issue #, if available:

None

Description of changes:

Updates ion-java to a commit of ion-tests that includes system macro and other test cases.
Implements the make_timestamp, delta, and sum system macros
Updates the repeat system macro to be aligned with the specification (i.e. allow a repeat count of 0, and allow repeating an empty stream)
Update all system macros to use tagged parameters
Conformance runner improvements:
- Change Annot -> annot (for compliance with the spec)
- Adds support for mactab fragments
- Adds support for demangling E-expressions in toplevel fragments

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

popematt · 2024-11-26T23:57:33Z

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

-                    }
-                    if (parameterPresence == PresenceBitmap.GROUP) {
+                    } else if (parameterPresence == PresenceBitmap.GROUP) {


🗺️ This was a bug I discovered that was causing + cardinality to be read incorrectly.

popematt · 2024-11-26T23:58:41Z

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

+                // Add an empty expression group if nothing present.
+                int index = expressions.size() + 1;
+                expressions.add(new Expression.ExpressionGroup(index, index));


🗺️ When there's nothing there, and we're expecting a parameter, we need to create a "nothing" expression. (In this case, it's an empty expression group, but I think that there's probably a good justification for introducing a NothingExpression.)

popematt · 2024-11-26T23:59:53Z

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

@@ -34,6 +34,8 @@ internal class IonManagedWriter_1_1(
    private val onClose: () -> Unit,
 ) : _Private_IonWriter, MacroAwareIonWriter {

+    internal fun getRawUserWriter(): IonRawWriter_1_1 = userData


🗺️ Hack so that I can convert mangled e-expressions in a toplevel fragment into serialized data.

codecov · 2024-12-04T01:06:03Z

Codecov Report

Attention: Patch coverage is 86.44068% with 16 lines in your changes missing coverage. Please review.

Please upload report for BASE (ion-11-encoding@44da868). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...n/java/com/amazon/ion/impl/macro/MacroEvaluator.kt	86.02%	5 Missing and 8 partials ⚠️
...mazon/ion/impl/IonReaderContinuableCoreBinary.java	0.00%	0 Missing and 1 partial ⚠️
.../main/java/com/amazon/ion/impl/macro/Expression.kt	50.00%	1 Missing ⚠️
...main/java/com/amazon/ion/impl/macro/SystemMacro.kt	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##             ion-11-encoding    #1003   +/-   ##
==================================================
  Coverage                   ?   71.07%           
  Complexity                 ?     7213           
==================================================
  Files                      ?      204           
  Lines                      ?    28554           
  Branches                   ?     5116           
==================================================
  Hits                       ?    20294           
  Misses                     ?     6677           
  Partials                   ?     1583

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

popematt · 2024-12-04T01:01:07Z

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

@@ -2348,7 +2348,7 @@ private void uncheckedReadMacroInvocationHeader(IonTypeID valueTid, Marker marke
    private boolean uncheckedReadHeader(final int typeIdByte, final boolean isAnnotated, final Marker markerToSet) {
        IonTypeID valueTid = typeIds[typeIdByte];
        if (!valueTid.isValid) {
-            throw new IonException("Invalid type ID.");
+            throw new IonException("Invalid type ID: " + valueTid.theByte);


🗺️ Minor improvement for ease of debugging.

popematt · 2024-12-04T01:05:35Z

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

-    @JvmStatic
-    fun exactlyOneFlexInt(name: String) = Parameter(name, ParameterEncoding.FlexInt, ParameterCardinality.ExactlyOne)


🗺️ This is now unused now that all system macros use tagged parameters.

popematt · 2024-12-04T01:10:22Z

src/test/java/com/amazon/ion/conformance/expectations.kt

@@ -356,7 +356,7 @@ private fun TestCaseSupport.denotesStruct(expectation: SeqElement, reader: IonRe
 private fun TestCaseSupport.denotesSymtok(expectation: AnyElement, actual: SymbolToken) {
    when (expectation) {
        is TextElement -> assertEquals(expectation.textValue, actual.text, createFailureMessage(expectation))
-        is IntElement -> assertEquals(expectation.longValue, actual.sid, createFailureMessage(expectation))
+        is IntElement -> assertEquals(expectation.longValue.toInt(), actual.sid, createFailureMessage(expectation))


🗺️ ...because 0 and 0L aren't equal when they are boxed, I guess.

popematt · 2024-12-04T01:16:14Z

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

-        val m = if (isQualifiedSystemMacro) SystemMacro.get(macroRef) else getMacro(macroRef)
+        val m = if (isQualifiedSystemMacro) SystemMacro.getMacroOrSpecialForm(macroRef) else getMacro(macroRef)


🗺️ This fixes a bug that prevented the macro compiler from recognizing special forms.

popematt · 2024-12-04T01:30:09Z

src/test/java/com/amazon/ion/conformance/ConformanceTestRunner.kt

-    additionalSkipFilter: (File, String) -> Boolean = { _, _ -> true }
+    /** A predicate that returns `true` iff the test case should be skipped. */
+    additionalSkipFilter: (File, String) -> Boolean = { _, _ -> false }


🗺️ I changed this because it was confusing when I went to use it for the incremental reader conformance tests.

tgregg · 2024-12-04T20:36:37Z

src/test/java/com/amazon/ion/conformance/fragments.kt

+                    // Write an expression group
+                    writer as IonManagedWriter_1_1
+                    val rawWriter = writer.getRawUserWriter()
+                    rawWriter.stepInExpressionGroup(usingLengthPrefix = true)


Do we need eventually need a way to exercise delimited expression groups here too...? This question is likely applicable in many places.

I don't think so because this is a toplevel fragment, which is supposed to be encoding-agnostic. Expression groups should have full test coverage from other test cases that use text or binary fragments so that we can precisely control which text tokens or bytes are present in the encoding.

tgregg · 2024-12-04T20:45:48Z

src/test/java/com/amazon/ion/conformance/ConformanceTestRunner.kt

+                "in binary with a user macro address" in completeTestName -> false
+
+            // FIXME: Timestamp should not allow an offset of +/-1440
+            "the offset argument must be less than 1440" in completeTestName -> false


Being able to skip individual tests in a file by name is super nice.

Macro implementations and conformance test fixes

07b01e1

popematt commented Nov 27, 2024

View reviewed changes

popematt and others added 3 commits December 3, 2024 12:08

Update ion-tests submodule

a563da1

More fixes

a1debeb

Merge branch 'ion-11-encoding' into macro-evaluation-fixes-temp

67ad344

Minor cleanup

0745be3

popematt commented Dec 4, 2024

View reviewed changes

popematt marked this pull request as ready for review December 4, 2024 01:31

popematt requested a review from tgregg December 4, 2024 01:31

tgregg approved these changes Dec 4, 2024

View reviewed changes

popematt merged commit 0a17f82 into amazon-ion:ion-11-encoding Dec 4, 2024
23 of 36 checks passed

tgregg pushed a commit that referenced this pull request Dec 13, 2024

Macro implementations and conformance test fixes (#1003)

c23b2d4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Macro implementations and conformance test fixes #1003

Macro implementations and conformance test fixes #1003

popematt commented Nov 26, 2024 •

edited

Loading

popematt Nov 26, 2024

popematt Nov 26, 2024

popematt Nov 26, 2024

codecov bot commented Dec 4, 2024 •

edited

Loading

popematt Dec 4, 2024

popematt Dec 4, 2024

popematt Dec 4, 2024

popematt Dec 4, 2024

popematt Dec 4, 2024

tgregg Dec 4, 2024

popematt Dec 4, 2024

tgregg Dec 4, 2024

		@JvmStatic
		fun exactlyOneFlexInt(name: String) = Parameter(name, ParameterEncoding.FlexInt, ParameterCardinality.ExactlyOne)

		val m = if (isQualifiedSystemMacro) SystemMacro.get(macroRef) else getMacro(macroRef)
		val m = if (isQualifiedSystemMacro) SystemMacro.getMacroOrSpecialForm(macroRef) else getMacro(macroRef)

Macro implementations and conformance test fixes #1003

Macro implementations and conformance test fixes #1003

Conversation

popematt commented Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

popematt commented Nov 26, 2024 •

edited

Loading

codecov bot commented Dec 4, 2024 •

edited

Loading