From f32e59f9d0803f55c414bfe85fa76c51c56c5d15 Mon Sep 17 00:00:00 2001 From: Matthew Pope Date: Mon, 12 Aug 2024 11:02:12 -0700 Subject: [PATCH] Try to isolate the problem --- .../ionelement/impl/IonElementLoaderImpl.kt | 194 +++++++++--------- .../ionelement/impl/StructElementImpl.kt | 2 +- .../ionelement/IonElementLoaderTests.kt | 72 ++++++- 3 files changed, 173 insertions(+), 95 deletions(-) diff --git a/src/main/kotlin/com/amazon/ionelement/impl/IonElementLoaderImpl.kt b/src/main/kotlin/com/amazon/ionelement/impl/IonElementLoaderImpl.kt index 7bf25a3..6b0a363 100644 --- a/src/main/kotlin/com/amazon/ionelement/impl/IonElementLoaderImpl.kt +++ b/src/main/kotlin/com/amazon/ionelement/impl/IonElementLoaderImpl.kt @@ -26,6 +26,7 @@ import com.amazon.ion.system.IonReaderBuilder import com.amazon.ionelement.api.* import com.amazon.ionelement.impl.collections.* import java.util.ArrayDeque +import java.util.ArrayList import kotlinx.collections.immutable.adapters.ImmutableListAdapter internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions) : IonElementLoader { @@ -75,12 +76,105 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions } override fun loadAllElements(ionReader: IonReader): List { - val elements = mutableListOf() - _loadAllElements(ionReader, elements as MutableList) - return elements.toImmutableListUnsafe() + return handleReaderException(ionReader) { + val elements = mutableListOf() + while (ionReader.next() != null) { + val depth = ionReader.depth + elements.add(loadCurrentElement(ionReader)) + check(depth == ionReader.depth) + } + elements + } } - fun _loadAllElements(ionReader: IonReader, into: MutableList) { + override fun loadAllElements(ionText: String): List = + IonReaderBuilder.standard().build(ionText).use(::loadAllElements) + + override fun loadCurrentElement(ionReader: IonReader): AnyElement { + return private_loadCurrentElement(ionReader) + } + + private fun private_loadCurrentElement(ionReader: IonReader): AnyElement { + return handleReaderException(ionReader) { + val valueType = requireNotNull(ionReader.type) { "The IonReader was not positioned at an element." } + + // Read a value + val annotations = ionReader.typeAnnotations!!.toImmutableListUnsafe() + + var metas = EMPTY_METAS + if (options.includeLocationMeta) { + val location = ionReader.currentLocation() + if (location != null) metas = location.toMetaContainer() + } + + val element = if (ionReader.isNullValue) { + ionNull(valueType.toElementType(), annotations, metas).asAnyElement() + } else when (valueType) { + IonType.BOOL -> BoolElementImpl(ionReader.booleanValue(), annotations, metas) + IonType.INT -> when (ionReader.integerSize!!) { + IntegerSize.BIG_INTEGER -> { + val bigIntValue = ionReader.bigIntegerValue() + // Ion java's IonReader appears to determine integerSize based on number of bits, + // not on the actual value, which means if we have a padded int that is > 63 bits, + // but whose value only uses <= 63 bits then integerSize is still BIG_INTEGER. + // Compensate for that here... + if (bigIntValue !in RANGE_OF_LONG) + BigIntIntElementImpl(bigIntValue, annotations, metas) + else { + LongIntElementImpl(ionReader.longValue(), annotations, metas) + } + } + + IntegerSize.LONG, + IntegerSize.INT -> LongIntElementImpl(ionReader.longValue(), annotations, metas) + } + + IonType.FLOAT -> FloatElementImpl(ionReader.doubleValue(), annotations, metas) + IonType.DECIMAL -> DecimalElementImpl(ionReader.decimalValue(), annotations, metas) + IonType.TIMESTAMP -> TimestampElementImpl(ionReader.timestampValue(), annotations, metas) + IonType.STRING -> StringElementImpl(ionReader.stringValue(), annotations, metas) + IonType.SYMBOL -> SymbolElementImpl(ionReader.stringValue(), annotations, metas) + IonType.CLOB -> ClobElementImpl(ionReader.newBytes(), annotations, metas) + IonType.BLOB -> BlobElementImpl(ionReader.newBytes(), annotations, metas) + IonType.LIST -> { + val listContent = mutableListOf() + val depth = ionReader.depth + ionReader.stepIn() + private_loadAllElements(ionReader, listContent as MutableList) + ionReader.stepOut() + check(depth == ionReader.depth) + ListElementImpl(listContent.toImmutableListUnsafe(), annotations, metas) + } + + IonType.SEXP -> { + val sexpContent = mutableListOf() + val depth = ionReader.depth + ionReader.stepIn() + private_loadAllElements(ionReader, sexpContent as MutableList) + ionReader.stepOut() + check(depth == ionReader.depth) + SexpElementImpl(sexpContent.toImmutableListUnsafe(), annotations, metas) + } + + IonType.STRUCT -> { + val structContent = mutableListOf() + val depth = ionReader.depth + ionReader.stepIn() + private_loadAllElements(ionReader, structContent as MutableList) + ionReader.stepOut() + check(depth == ionReader.depth) + StructElementImpl(structContent.toImmutableListUnsafe(), annotations, metas) + } + + IonType.DATAGRAM -> error("IonElementLoaderImpl does not know what to do with IonType.DATAGRAM") + IonType.NULL -> error("IonType.NULL branch should be unreachable") + + }.asAnyElement() + element + } + } + + private fun private_loadAllElements(ionReader: IonReader, into: MutableList) { // Intentionally not a recycling stack because we have mutable references that we are going to wrap as an // ImmutableList and then forget about the reference to the mutable list. val sequenceContentStack = ArrayDeque>() @@ -94,7 +188,7 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions // End of container or input if (valueType == null) { - if (sequenceContentStack.isEmpty()) { + if (sequenceContentStack.isEmpty() && fieldsStack.isEmpty()) { return } else { ionReader.stepOut() @@ -147,19 +241,19 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions IonType.CLOB -> ClobElementImpl(ionReader.newBytes(), annotations, metas) IonType.BLOB -> BlobElementImpl(ionReader.newBytes(), annotations, metas) IonType.LIST -> { - val listContent = mutableListOf() + val listContent = ArrayList() nextElements = listContent as MutableList ListElementImpl(ImmutableListAdapter(listContent), annotations, metas) } IonType.SEXP -> { - val sexpContent = mutableListOf() + val sexpContent = ArrayList() nextElements = sexpContent as MutableList SexpElementImpl(ImmutableListAdapter(sexpContent), annotations, metas) } IonType.STRUCT -> { - val structContent = mutableListOf() + val structContent = ArrayList() nextElements = structContent as MutableList StructElementImpl(ImmutableListAdapter(structContent), annotations, metas) } @@ -195,88 +289,4 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions ) } } - - fun _loadCurrentElement(ionReader: IonReader): AnyElement { - // Intentionally not a recycling stack because we have mutable references that we are going to wrap as an - // ImmutableList and then forget about the reference to the mutable list. - - return handleReaderException(ionReader) { - val valueType = requireNotNull(ionReader.type) { "The IonReader was not positioned at an element." } - - // Read a value - val annotations = ionReader.typeAnnotations!!.toImmutableListUnsafe() - - var metas = EMPTY_METAS - if (options.includeLocationMeta) { - val location = ionReader.currentLocation() - if (location != null) metas = location.toMetaContainer() - } - - val element = if (ionReader.isNullValue) { - ionNull(valueType.toElementType(), annotations, metas).asAnyElement() - } else when (valueType) { - IonType.BOOL -> BoolElementImpl(ionReader.booleanValue(), annotations, metas) - IonType.INT -> when (ionReader.integerSize!!) { - IntegerSize.BIG_INTEGER -> { - val bigIntValue = ionReader.bigIntegerValue() - // Ion java's IonReader appears to determine integerSize based on number of bits, - // not on the actual value, which means if we have a padded int that is > 63 bits, - // but whose value only uses <= 63 bits then integerSize is still BIG_INTEGER. - // Compensate for that here... - if (bigIntValue !in RANGE_OF_LONG) - BigIntIntElementImpl(bigIntValue, annotations, metas) - else { - LongIntElementImpl(ionReader.longValue(), annotations, metas) - } - } - - IntegerSize.LONG, - IntegerSize.INT -> LongIntElementImpl(ionReader.longValue(), annotations, metas) - } - - IonType.FLOAT -> FloatElementImpl(ionReader.doubleValue(), annotations, metas) - IonType.DECIMAL -> DecimalElementImpl(ionReader.decimalValue(), annotations, metas) - IonType.TIMESTAMP -> TimestampElementImpl(ionReader.timestampValue(), annotations, metas) - IonType.STRING -> StringElementImpl(ionReader.stringValue(), annotations, metas) - IonType.SYMBOL -> SymbolElementImpl(ionReader.stringValue(), annotations, metas) - IonType.CLOB -> ClobElementImpl(ionReader.newBytes(), annotations, metas) - IonType.BLOB -> BlobElementImpl(ionReader.newBytes(), annotations, metas) - IonType.LIST -> { - val listContent = mutableListOf() - ionReader.stepIn() - _loadAllElements(ionReader, listContent as MutableList) - ionReader.stepOut() - ListElementImpl(listContent.toImmutableListUnsafe(), annotations, metas) - } - - IonType.SEXP -> { - val sexpContent = mutableListOf() - ionReader.stepIn() - _loadAllElements(ionReader, sexpContent as MutableList) - ionReader.stepOut() - SexpElementImpl(sexpContent.toImmutableListUnsafe(), annotations, metas) - } - - IonType.STRUCT -> { - val structContent = mutableListOf() - ionReader.stepIn() - _loadAllElements(ionReader, structContent as MutableList) - ionReader.stepOut() - StructElementImpl(structContent.toImmutableListUnsafe(), annotations, metas) - } - - IonType.DATAGRAM -> error("IonElementLoaderImpl does not know what to do with IonType.DATAGRAM") - IonType.NULL -> error("IonType.NULL branch should be unreachable") - - }.asAnyElement() - element - } - } - - override fun loadAllElements(ionText: String): List = - IonReaderBuilder.standard().build(ionText).use(::loadAllElements) - - override fun loadCurrentElement(ionReader: IonReader): AnyElement { - return _loadCurrentElement(ionReader) - } } diff --git a/src/main/kotlin/com/amazon/ionelement/impl/StructElementImpl.kt b/src/main/kotlin/com/amazon/ionelement/impl/StructElementImpl.kt index a133dd1..cef15f3 100644 --- a/src/main/kotlin/com/amazon/ionelement/impl/StructElementImpl.kt +++ b/src/main/kotlin/com/amazon/ionelement/impl/StructElementImpl.kt @@ -31,7 +31,7 @@ internal class StructElementImpl( ) : AnyElementBase(), StructElement { override val type: ElementType get() = ElementType.STRUCT - override val size = allFields.size + override val size: Int get() = allFields.size // Note that we are not using `by lazy` here because it requires 2 additional allocations and // has been demonstrated to significantly increase memory consumption! diff --git a/src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt b/src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt index 01e183d..7a7bf08 100644 --- a/src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt +++ b/src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt @@ -25,6 +25,7 @@ import com.amazon.ionelement.util.convertToString import kotlinx.collections.immutable.adapters.ImmutableListAdapter import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Disabled import org.junit.jupiter.api.Test import org.junit.jupiter.params.ParameterizedTest import org.junit.jupiter.params.provider.MethodSource @@ -100,7 +101,11 @@ class IonElementLoaderTests { IonElementLoaderTestCase("{foo:{bar:1}}", ionStructOf("foo" to ionStructOf("bar" to ionInt(1)))), IonElementLoaderTestCase("{foo:[{}]}", ionStructOf("foo" to ionListOf(ionStructOf(emptyList())))), IonElementLoaderTestCase("[{}]", ionListOf(ionStructOf(emptyList()))), + IonElementLoaderTestCase("[{}, {}]", ionListOf(ionStructOf(emptyList()), ionStructOf(emptyList()))), IonElementLoaderTestCase("[{foo:1, bar: 2}]", ionListOf(ionStructOf("foo" to ionInt(1), "bar" to ionInt(2)))), + IonElementLoaderTestCase("{foo:[{bar:[{}]}]}", + ionStructOf("foo" to ionListOf(ionStructOf("bar" to ionListOf(ionStructOf(emptyList())))))), + ) } @@ -124,6 +129,27 @@ class IonElementLoaderTests { @Test fun parsedAndConstructedInstances() { + assertEquals( + loadSingleElement("[1, a, true]"), + ionListOf(ionInt(1), ionSymbol("a"), ionBool(true)) + ) + assertEquals( + loadSingleElement("{foo:1}"), + ionStructOf("foo" to ionInt(1)) + ) + + + val struct1 = loadSingleElement("{foo:1}") + // Passes + assertEquals( + ionListOf(struct1), + ionListOf(ionStructOf("foo" to ionInt(1))) + ) + // Fails + assertEquals( + ionListOf(struct1), + loadSingleElement("[{foo:1}]"), + ) assertEquals( loadSingleElement("[{foo:1}]"), ionListOf(ionStructOf("foo" to ionInt(1))) @@ -138,6 +164,50 @@ class IonElementLoaderTests { ) } + @Test + fun puzzling() { + val parsedList = loadSingleElement("[foo, 1]").asList() + val constructedList = ionListOf(ionSymbol("foo"), ionInt(1)) + val parsedStruct = loadSingleElement("{foo:1}").asStruct() + val constructedStruct = ionStructOf("foo" to ionInt(1)) + val parsedListOfStruct = loadSingleElement("[{foo:1}]").asList() + val constructedListOfStruct = ionListOf(ionStructOf("foo" to ionInt(1))) + + println("Parsed vs Constructed") + // TRUE + println(parsedList == constructedList) + println(constructedList == parsedList) + println(parsedStruct == constructedStruct) + println(constructedStruct == parsedStruct) + // FALSE + println(parsedListOfStruct == constructedListOfStruct) + println(constructedListOfStruct == parsedListOfStruct) + + println("Mix of parsed and constructed") + + // True + println(constructedListOfStruct == ionListOf(parsedStruct)) + println(constructedListOfStruct == ionListOf(constructedStruct)) + + // False + println(parsedListOfStruct == ionListOf(parsedStruct)) + println(parsedListOfStruct == ionListOf(constructedStruct)) + println(ionListOf(parsedStruct) == parsedListOfStruct) + println(ionListOf(constructedStruct) == parsedListOfStruct) + + // True + println(ionListOf(parsedStruct) == ionListOf(constructedStruct)) + + // True + println(parsedListOfStruct == loadSingleElement(" [{foo:1}]")) + + println("As Strings") + // true + println("$constructedList" == "$parsedList") + println("$constructedStruct" == "$parsedStruct") + println("$constructedListOfStruct" == "$parsedListOfStruct") + } + @Test fun kotlinIdiomaticTest2() { @@ -174,7 +244,5 @@ class IonElementLoaderTests { parsedIonElement.asAnyElement().asList().values[0].asStruct() ) // assertEquals(tc.expectedElement, parsedIonElement) - - // assertEquals(tc.expectedElement, parsedIonElement) } }