Skip to content

Commit

Permalink
Adds loader option to use iterative-only loading
Browse files Browse the repository at this point in the history
  • Loading branch information
popematt committed Sep 25, 2024
1 parent 9ede0c8 commit aa3ce1d
Show file tree
Hide file tree
Showing 6 changed files with 207 additions and 16 deletions.
21 changes: 21 additions & 0 deletions api/IonElement.api
Original file line number Diff line number Diff line change
Expand Up @@ -388,18 +388,39 @@ public final class com/amazon/ionelement/api/IonElementLoaderException : com/ama
}

public final class com/amazon/ionelement/api/IonElementLoaderOptions {
public static final field Companion Lcom/amazon/ionelement/api/IonElementLoaderOptions$Companion;
public fun <init> ()V
public fun <init> (Z)V
public synthetic fun <init> (ZILkotlin/jvm/internal/DefaultConstructorMarker;)V
public static final fun builder ()Lcom/amazon/ionelement/api/IonElementLoaderOptions$Builder;
public final fun component1 ()Z
public final fun copy ()Lcom/amazon/ionelement/api/IonElementLoaderOptions;
public final fun copy (Z)Lcom/amazon/ionelement/api/IonElementLoaderOptions;
public static synthetic fun copy$default (Lcom/amazon/ionelement/api/IonElementLoaderOptions;ZILjava/lang/Object;)Lcom/amazon/ionelement/api/IonElementLoaderOptions;
public final synthetic fun copyWith (Lkotlin/jvm/functions/Function1;)Lcom/amazon/ionelement/api/IonElementLoaderOptions;
public fun equals (Ljava/lang/Object;)Z
public final fun getIncludeLocationMeta ()Z
public final fun getUseRecursiveLoad ()Z
public fun hashCode ()I
public final fun toBuilder ()Lcom/amazon/ionelement/api/IonElementLoaderOptions$Builder;
public fun toString ()Ljava/lang/String;
}

public final class com/amazon/ionelement/api/IonElementLoaderOptions$Builder {
public final fun build ()Lcom/amazon/ionelement/api/IonElementLoaderOptions;
public final fun getIncludeLocationMeta ()Z
public final fun getUseRecursiveLoad ()Z
public final synthetic fun setIncludeLocationMeta (Z)V
public final synthetic fun setUseRecursiveLoad (Z)V
public final fun withIncludeLocationMeta (Z)Lcom/amazon/ionelement/api/IonElementLoaderOptions$Builder;
public final fun withUseRecursiveLoad (Z)Lcom/amazon/ionelement/api/IonElementLoaderOptions$Builder;
}

public final class com/amazon/ionelement/api/IonElementLoaderOptions$Companion {
public final fun builder ()Lcom/amazon/ionelement/api/IonElementLoaderOptions$Builder;
public final synthetic fun invoke (Lkotlin/jvm/functions/Function1;)Lcom/amazon/ionelement/api/IonElementLoaderOptions;
}

public abstract class com/amazon/ionelement/api/IonLocation {
}

Expand Down
144 changes: 136 additions & 8 deletions src/main/kotlin/com/amazon/ionelement/api/IonElementLoader.kt
Original file line number Diff line number Diff line change
Expand Up @@ -76,17 +76,145 @@ public interface IonElementLoader {
/**
* Specifies options for [IonElementLoader].
*
* While there is only one property here currently, new properties may be added to this class without breaking
* source compatibility with prior versions of this library.
* Java consumers must use [IonElementLoaderOptions.builder] to create an instance of [IonElementLoaderOptions].
* ```java
* IonElementLoaderOptions loaderOpts = IonElementLoaderOptions.builder()
* .withIncludeLocationMetadata(true)
* .build();
* ```
* When calling from Kotlin, [IonElementLoaderOptions.invoke] is also available.
* ```kotlin
* val loaderOpts = IonElementLoaderOptions {
* includeLocationMetadata = true
* }
* ```
*/
public data class IonElementLoaderOptions(
/**
* Set to true to cause `IonLocation` to be stored in the [IonElement.metas] collection of all elements loaded.
public class IonElementLoaderOptions internal constructor(
val includeLocationMeta: Boolean,
val useRecursiveLoad: Boolean,
) {
/*
* Intentionally not a KDoc comment.
*
* IonElementLoaderOptions used to be a data class, but data classes have some inherent backwards compatibility
* issues. Namely, that the auto-generated `copy` function does not have Java overloads with lesser numbers of
* parameters.
*
* This is `false` by default because it has a performance penalty.
* The `copy()` and `component1()` functions and the single arg constructor in this class exist for backwards
* compatibility. They should be removed in the next major version.
*/

@Deprecated("Will be removed in the next major version. Replace with builder.")
@JvmOverloads
constructor(includeLocationMeta: Boolean = false) : this(includeLocationMeta, DEFAULT.useRecursiveLoad)

@Deprecated("Will be removed in the next major version. Use toBuilder() to copy this to a new builder.")
@JvmOverloads
fun copy(includeLocationMeta: Boolean = this.includeLocationMeta): IonElementLoaderOptions {
return IonElementLoaderOptions(includeLocationMeta, useRecursiveLoad)
}

@Deprecated("Will be removed in the next major version. Replace with getIncludeLocationMetadata().")
public operator fun component1() = includeLocationMeta

/**
* Returns a new [Builder] that is prepopulated with the values in this [IonElementLoaderOptions].
*/
fun toBuilder(): Builder = Builder(this)

/**
* Creates a new copy of this [IonElementLoaderOptions] with the given changes.
*/
@JvmSynthetic
fun copyWith(block: Builder.() -> Unit) = toBuilder().apply(block).build()

override fun equals(other: Any?): Boolean {
if (this === other) return true
if (other !is IonElementLoaderOptions) return false
return includeLocationMeta == other.includeLocationMeta &&
useRecursiveLoad == other.useRecursiveLoad
}

override fun hashCode(): Int {
// We can treat all the boolean options as flags in a bitfield to guarantee no hash collisions.
return (if (includeLocationMeta) 1 else 0) +
(if (useRecursiveLoad) 2 else 0)
}

override fun toString(): String {
return "IonElementLoaderOptions(" +
"includeLocationMeta=$includeLocationMeta," +
"useRecursiveLoad=$useRecursiveLoad," +
")"
}

companion object {
@JvmStatic
private val DEFAULT = IonElementLoaderOptions(
includeLocationMeta = false,
useRecursiveLoad = true,
)

@JvmStatic
fun builder() = Builder(DEFAULT)

@JvmSynthetic
operator fun invoke(block: Builder.() -> Unit): IonElementLoaderOptions {
return builder().apply(block).build()
}
}

/*
* Implementation Note:
* This builder includes `with*` methods for idiomatic usage from Java, and the properties have their setters marked
* with `@JvmSynthetic` rather than private so that they are visible from Kotlin for a DSL-like experience.
*/
val includeLocationMeta: Boolean = false
)
class Builder internal constructor(startingValues: IonElementLoaderOptions) {
/**
* Set to `true` to cause `IonLocation` to be stored in the [IonElement.metas] collection of all elements loaded.
*
* This is `false` by default because it has a performance penalty.
*/
var includeLocationMeta: Boolean = startingValues.includeLocationMeta
@JvmSynthetic set

/**
* Set to `false` to cause the [IonElementLoader] to use an iterative loader. Otherwise, the loader will use a
* recursive approach and fall back to the iterative loader if it steps into nested containers with a depth greater
* than 100.
*
* This is `true` by default.
*
* This does not affect the behavior of the [IonElementLoader] other than its performance characteristics. If
* performance is critical, users should benchmark both options for loading typical data and select the one with
* the desired performance characteristics.
*/
var useRecursiveLoad: Boolean = startingValues.useRecursiveLoad
@JvmSynthetic set

/**
* Set to `true` to cause `IonLocation` to be stored in the [IonElement.metas] collection of all elements loaded.
*
* This is `false` by default because it has a performance penalty.
*/
fun withIncludeLocationMeta(value: Boolean) = apply { includeLocationMeta = value }

/**
* Set to `false` to cause the [IonElementLoader] to use an iterative loader. Otherwise, the loader will use a
* recursive approach and fall back to the iterative loader if it steps into nested containers with a depth greater
* than 100.
*
* This is `true` by default.
*
* This does not affect the behavior of the [IonElementLoader] other than its performance characteristics. If
* performance is critical, users should benchmark both options for loading typical data and select the one with
* the desired performance characteristics.
*/
fun withUseRecursiveLoad(value: Boolean) = apply { useRecursiveLoad = value }

fun build() = IonElementLoaderOptions(includeLocationMeta, useRecursiveLoad)
}
}

/** Creates an [IonElementLoader] implementation with the specified [options]. */
@JvmOverloads
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,12 @@ import kotlinx.collections.immutable.adapters.ImmutableListAdapter

internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions) : IonElementLoader {

// TODO: It seems like some data can be read faster with a recursive approach, but other data is
// faster with an iterative approach. Consider making this configurable. It probably doesn't
// need to be finely-tune-able—just 0 or 100 (i.e. on/off) is probably sufficient.
companion object {
private const val MAX_RECURSION_DEPTH: Int = 100
private const val DEFAULT_MAX_RECURSION_DEPTH: Int = 100
}

private var maxRecursionDepth = if (options.useRecursiveLoad) DEFAULT_MAX_RECURSION_DEPTH else 0

/**
* Catches an [IonException] occurring in [block] and throws an [IonElementLoaderException] with
* the current [IonLocation] of the fault, if one is available. Note that depending on the state of the
Expand Down Expand Up @@ -143,7 +142,7 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions
IonType.LIST -> {
ionReader.stepIn()
val listContent = ArrayList<AnyElement>()
if (ionReader.depth < MAX_RECURSION_DEPTH) {
if (ionReader.depth < maxRecursionDepth) {
while (ionReader.next() != null) {
listContent.add(loadCurrentElementRecursively(ionReader))
}
Expand All @@ -156,7 +155,7 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions
IonType.SEXP -> {
ionReader.stepIn()
val sexpContent = ArrayList<AnyElement>()
if (ionReader.depth < MAX_RECURSION_DEPTH) {
if (ionReader.depth < maxRecursionDepth) {
while (ionReader.next() != null) {
sexpContent.add(loadCurrentElementRecursively(ionReader))
}
Expand All @@ -169,7 +168,7 @@ internal class IonElementLoaderImpl(private val options: IonElementLoaderOptions
IonType.STRUCT -> {
val fields = ArrayList<StructField>()
ionReader.stepIn()
if (ionReader.depth < MAX_RECURSION_DEPTH) {
if (ionReader.depth < maxRecursionDepth) {
while (ionReader.next() != null) {
val fieldName = ionReader.fieldName
val element = loadCurrentElementRecursively(ionReader)
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/com/amazon/ionelement/demos/ElementLoaderDemo.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.amazon.ion.IonReader;
import com.amazon.ionelement.api.AnyElement;
import com.amazon.ionelement.api.ElementLoader;
import com.amazon.ionelement.api.IonElementLoaderOptions;
import com.amazon.ionelement.util.TestUtils;
import org.junit.jupiter.api.Test;

Expand All @@ -17,6 +18,26 @@ public class ElementLoaderDemo extends DemoBase {
private static final String ION_TEXT_STRUCT = "{ a_field: \"hello!\"}";
private static final String ION_TEXT_MULTIPLE_VALUES = "1 2 3";

@Test
void createIonElementLoaderOptions() {
IonElementLoaderOptions opts = IonElementLoaderOptions.builder()
.withIncludeLocationMeta(true)
.withUseRecursiveLoad(false)
.build();

assertTrue(opts.getIncludeLocationMeta());
assertFalse(opts.getUseRecursiveLoad());

IonElementLoaderOptions copy = opts.toBuilder()
.withUseRecursiveLoad(true)
.build();

// Unchanged
assertTrue(copy.getIncludeLocationMeta());
// Changed
assertTrue(copy.getUseRecursiveLoad());
}

@Test
void loadSingleElement_text() {
AnyElement value = ElementLoader.loadSingleElement(ION_TEXT_STRUCT);
Expand Down
22 changes: 22 additions & 0 deletions src/test/kotlin/com/amazon/ionelement/IonElementLoaderTests.kt
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import com.amazon.ionelement.util.IonElementLoaderTestCase
import com.amazon.ionelement.util.convertToString
import java.math.BigInteger
import org.junit.jupiter.api.Assertions.assertEquals
import org.junit.jupiter.api.Assertions.assertFalse
import org.junit.jupiter.api.Assertions.assertTrue
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.assertThrows
import org.junit.jupiter.params.ParameterizedTest
Expand Down Expand Up @@ -170,4 +172,24 @@ class IonElementLoaderTests {
fun `loadSingleElement throws exception when more than one values in reader`() {
assertThrows<IllegalArgumentException> { loadSingleElement("a b") }
}

@Test
fun `create an IonElementLoaderOptions instance`() {
val opts = IonElementLoaderOptions {
includeLocationMeta = true
useRecursiveLoad = false
}

assertTrue(opts.includeLocationMeta)
assertFalse(opts.useRecursiveLoad)

val copy = opts.copyWith {
useRecursiveLoad = true
}

// Unchanged
assertTrue(copy.includeLocationMeta)
// Changed
assertTrue(copy.useRecursiveLoad)
}
}
2 changes: 1 addition & 1 deletion src/test/kotlin/com/amazon/ionelement/util/TestUtils.kt
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@ fun convertToString(ionValue: IonValue): String {
*
* This is to support this somewhat commonly used scenario.
*/
val INCLUDE_LOCATION_META = IonElementLoaderOptions(includeLocationMeta = true)
val INCLUDE_LOCATION_META = IonElementLoaderOptions { includeLocationMeta = true }

0 comments on commit aa3ce1d

Please sign in to comment.