From 715852c87afd8e6276eb04532032609c14f3b8a6 Mon Sep 17 00:00:00 2001 From: Matthew Pope Date: Wed, 7 Aug 2024 17:24:03 -0700 Subject: [PATCH] Iterative implementation of loadAllElements --- .../ionelement/impl/IonElementLoaderImpl.kt | 169 ++++++++++++++++-- .../ionelement/impl/StructElementImpl.kt | 2 +- .../ionelement/impl/collections/extensions.kt | 37 ++++ .../ionelement/IonElementLoaderTests.kt | 26 ++- 4 files changed, 211 insertions(+), 23 deletions(-) diff --git a/src/main/kotlin/com/amazon/ionelement/impl/IonElementLoaderImpl.kt b/src/main/kotlin/com/amazon/ionelement/impl/IonElementLoaderImpl.kt index 997e269..6b0a363 100644 --- a/src/main/kotlin/com/amazon/ionelement/impl/IonElementLoaderImpl.kt +++ b/src/main/kotlin/com/amazon/ionelement/impl/IonElementLoaderImpl.kt @@ -25,6 +25,9 @@ import com.amazon.ion.TextSpan 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 { @@ -76,7 +79,9 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions return handleReaderException(ionReader) { val elements = mutableListOf() while (ionReader.next() != null) { + val depth = ionReader.depth elements.add(loadCurrentElement(ionReader)) + check(depth == ionReader.depth) } elements } @@ -86,9 +91,14 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions 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 @@ -97,10 +107,9 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions if (location != null) metas = location.toMetaContainer() } - if (ionReader.isNullValue) { - ionNull(valueType.toElementType(), annotations, metas) - } else { - when (valueType) { + 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 -> { @@ -115,6 +124,7 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions LongIntElementImpl(ionReader.longValue(), annotations, metas) } } + IntegerSize.LONG, IntegerSize.INT -> LongIntElementImpl(ionReader.longValue(), annotations, metas) } @@ -127,35 +137,156 @@ 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 depth = ionReader.depth ionReader.stepIn() - val list = ListElementImpl(loadAllElements(ionReader).toImmutableListUnsafe(), annotations, metas) + private_loadAllElements(ionReader, listContent as MutableList) ionReader.stepOut() - list + check(depth == ionReader.depth) + ListElementImpl(listContent.toImmutableListUnsafe(), annotations, metas) } + IonType.SEXP -> { + val sexpContent = mutableListOf() + val depth = ionReader.depth ionReader.stepIn() - val sexp = SexpElementImpl(loadAllElements(ionReader).toImmutableListUnsafe(), annotations, metas) + private_loadAllElements(ionReader, sexpContent as MutableList) ionReader.stepOut() - sexp + check(depth == ionReader.depth) + SexpElementImpl(sexpContent.toImmutableListUnsafe(), annotations, metas) } + IonType.STRUCT -> { - val fields = mutableListOf() + val structContent = mutableListOf() + val depth = ionReader.depth ionReader.stepIn() - while (ionReader.next() != null) { - fields.add( - StructFieldImpl( - ionReader.fieldName, - loadCurrentElement(ionReader) - ) - ) - } + 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>() + val fieldsStack = ArrayDeque>() + var elements: MutableList = into + var nextElements: MutableList = elements + + try { + while (true) { + val valueType = ionReader.next() + + // End of container or input + if (valueType == null) { + if (sequenceContentStack.isEmpty() && fieldsStack.isEmpty()) { + return + } else { ionReader.stepOut() - StructElementImpl(fields.toImmutableListUnsafe(), annotations, metas) + nextElements = if (ionReader.isInStruct) { + fieldsStack.pop() as MutableList + } else { + sequenceContentStack.pop() as MutableList + } + elements = nextElements + continue + } + } + + // 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 = ArrayList() + nextElements = listContent as MutableList + ListElementImpl(ImmutableListAdapter(listContent), annotations, metas) + } + + IonType.SEXP -> { + val sexpContent = ArrayList() + nextElements = sexpContent as MutableList + SexpElementImpl(ImmutableListAdapter(sexpContent), annotations, metas) } + + IonType.STRUCT -> { + val structContent = ArrayList() + nextElements = structContent as MutableList + StructElementImpl(ImmutableListAdapter(structContent), 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() + + if (ionReader.isInStruct) { + elements.add(StructFieldImpl(ionReader.fieldName, element)) + } else { + elements.add(element) + } + + // Step in, if necessary + if (IonType.isContainer(valueType)) { + if (ionReader.isInStruct) { + fieldsStack.push(elements as MutableList) + } else { + sequenceContentStack.push(elements as MutableList) + } + ionReader.stepIn() } - }.asAnyElement() + elements = nextElements + } + + } catch (e: IonException) { + throw IonElementException( + location = ionReader.currentLocation(), + description = "IonException occurred, likely due to malformed Ion data (see cause)", + cause = e + ) } } } diff --git a/src/main/kotlin/com/amazon/ionelement/impl/StructElementImpl.kt b/src/main/kotlin/com/amazon/ionelement/impl/StructElementImpl.kt index 6a236b8..38f81e1 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/main/kotlin/com/amazon/ionelement/impl/collections/extensions.kt b/src/main/kotlin/com/amazon/ionelement/impl/collections/extensions.kt index dc73c60..7c0741a 100644 --- a/src/main/kotlin/com/amazon/ionelement/impl/collections/extensions.kt +++ b/src/main/kotlin/com/amazon/ionelement/impl/collections/extensions.kt @@ -75,6 +75,15 @@ internal fun List.toImmutableListUnsafe(): ImmutableList { return ImmutableListAdapter(this) } +/** + * Creates a [ImmutableList] for [this] without making a defensive copy. + * Only call this method if you are sure that [this] cannot leak anywhere it could be mutated. + */ +internal fun List.toImmutableListUnsafeAllowModification(): ImmutableList { + if (this is ImmutableList) return this + return ImmutableListAdapter(this) +} + /** * Creates a [ImmutableList] for [this] without making a defensive copy. * Only call this method if you are sure that [this] cannot leak anywhere it could be mutated. @@ -85,3 +94,31 @@ internal fun Array.toImmutableListUnsafe(): ImmutableList { // even further but using only a single wrapper layer, if we created such a thing. return ImmutableListAdapter(asList()) } + + +internal interface SometimesMutableList { + fun add(element: E) + fun seal(): ImmutableList +} + +private class SometimesMutableListImpl private constructor(private val impl: MutableList) : SometimesMutableList, ImmutableList, List by impl { + constructor(): this(mutableListOf()) + + private var isClosed = false + + override fun add(element: E) { + if (isClosed) throw UnsupportedOperationException() + impl.add(element) + } + + override fun seal(): ImmutableList { + isClosed = true + return this + } + + override fun subList(fromIndex: Int, toIndex: Int): ImmutableList = ImmutableListAdapter(impl.subList(fromIndex, toIndex)) + + override fun equals(other: Any?): Boolean = impl.equals(other) + override fun hashCode(): Int = impl.hashCode() + override fun toString(): String = impl.toString() +} diff --git a/src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt b/src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt index ca6e946..f714fc8 100644 --- a/src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt +++ b/src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt @@ -15,12 +15,18 @@ package com.amazon.ionelement +import com.amazon.ion.util.Equivalence import com.amazon.ionelement.api.* +import com.amazon.ionelement.impl.collections.* import com.amazon.ionelement.util.INCLUDE_LOCATION_META import com.amazon.ionelement.util.ION import com.amazon.ionelement.util.IonElementLoaderTestCase 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 @@ -45,6 +51,8 @@ class IonElementLoaderTests { // Converting from IonValue to IonElement should result in an IonElement that is equivalent to the // parsed IonElement assertEquals(parsedIonValue.toIonElement(), parsedIonElement) + + assertEquals(tc.expectedElement, parsedIonElement) } companion object { @@ -57,7 +65,7 @@ class IonElementLoaderTests { IonElementLoaderTestCase("1", ionInt(1)), - IonElementLoaderTestCase("existence::42", ionInt(1).withAnnotations("existence")), + IonElementLoaderTestCase("existence::42", ionInt(42).withAnnotations("existence")), IonElementLoaderTestCase("\"some string\"", ionString("some string")), @@ -65,7 +73,7 @@ class IonElementLoaderTests { IonElementLoaderTestCase("[1, 2, 3]", ionListOf(ionInt(1), ionInt(2), ionInt(3))), - IonElementLoaderTestCase("(1 2 3)", ionListOf(ionInt(1), ionInt(2), ionInt(3))), + IonElementLoaderTestCase("(1 2 3)", ionSexpOf(ionInt(1), ionInt(2), ionInt(3))), IonElementLoaderTestCase( "{ foo: 1, bar: 2, bat: 3 }", @@ -74,7 +82,19 @@ class IonElementLoaderTests { "bar" to ionInt(2), "bat" to ionInt(3) ) - ) + ), + + // Nested container cases + IonElementLoaderTestCase("(1 (2 3))", ionSexpOf(ionInt(1), ionSexpOf(ionInt(2), ionInt(3)))), + IonElementLoaderTestCase("{foo:[1]}", ionStructOf("foo" to ionListOf(ionInt(1)))), + IonElementLoaderTestCase("[{foo:1}]", ionListOf(ionStructOf("foo" to ionInt(1)))), + 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 ionSexpOf(ionStructOf(emptyList())))))), ) } }