From 153bd9f56f3c7f1d5e6ce1a830706b3d4a2b1e81 Mon Sep 17 00:00:00 2001 From: Eric Chen Date: Fri, 22 Oct 2021 15:57:04 -0700 Subject: [PATCH] Works on feedback, adds comments and improves tests. --- .../com/amazon/ionhiveserde/IonHiveSerDe.java | 8 +++ .../IonSequenceCaseInsensitiveDecorator.java | 45 +++++++--------- .../IonStructCaseInsensitiveDecorator.java | 6 +++ .../CaseInsensitiveDecoratorTest.kt | 51 ++++++++++--------- 4 files changed, 60 insertions(+), 50 deletions(-) diff --git a/serde/src/main/java/com/amazon/ionhiveserde/IonHiveSerDe.java b/serde/src/main/java/com/amazon/ionhiveserde/IonHiveSerDe.java index 8cec7fa..3f41079 100644 --- a/serde/src/main/java/com/amazon/ionhiveserde/IonHiveSerDe.java +++ b/serde/src/main/java/com/amazon/ionhiveserde/IonHiveSerDe.java @@ -136,6 +136,14 @@ public Object deserialize(final Writable blob) throws SerDeException { final IonSystem domFactory = ionFactory.getDomFactory(); try (final IonReader reader = ionFactory.newReader(bytes, 0, length)) { + + /* + We're using an IonStruct here because: + 1. We need a key-value store to carry column values + 2. The top-level IonStruct as a context object carries the IonSystem which we use as a ValueFactory in + the callbacks created in PathExtractionConfig + Refer to https://github.com/amzn/ion-hive-serde/issues/61. + */ IonStruct struct = domFactory.newEmptyStruct(); if (!serDeProperties.pathExtractorCaseSensitivity()) { struct = new IonStructCaseInsensitiveDecorator(struct); diff --git a/serde/src/main/java/com/amazon/ionhiveserde/caseinsensitivedecorator/IonSequenceCaseInsensitiveDecorator.java b/serde/src/main/java/com/amazon/ionhiveserde/caseinsensitivedecorator/IonSequenceCaseInsensitiveDecorator.java index 76dd93f..5e6ae20 100644 --- a/serde/src/main/java/com/amazon/ionhiveserde/caseinsensitivedecorator/IonSequenceCaseInsensitiveDecorator.java +++ b/serde/src/main/java/com/amazon/ionhiveserde/caseinsensitivedecorator/IonSequenceCaseInsensitiveDecorator.java @@ -22,10 +22,12 @@ import com.amazon.ion.UnknownSymbolException; import com.amazon.ion.ValueFactory; -import java.util.ArrayList; +import java.lang.reflect.Array; import java.util.Collection; import java.util.List; import java.util.ListIterator; +import java.util.stream.Collectors; + import org.jetbrains.annotations.NotNull; @@ -135,48 +137,37 @@ public ListIterator listIterator(final int index) { @NotNull @Override public List subList(final int fromIndex, final int toIndex) { - List l = new ArrayList<>(); - List sublist = ionSequence.subList(fromIndex, toIndex); - - for (IonValue ionValue : sublist) { - l.add(IonCaseInsensitiveDecorator.wrapValue(ionValue)); - } - return l; + return ionSequence.subList(fromIndex, toIndex).stream() + .map(IonCaseInsensitiveDecorator::wrapValue) + .collect(Collectors.toList()); } @NotNull @Override public IonValue[] toArray() { - IonValue[] rawArray = ionSequence.toArray(); - int size = ionSequence.size(); - for (int i = 0; i < size; i++) { - rawArray[i] = IonCaseInsensitiveDecorator.wrapValue(rawArray[i]); - } - return rawArray; + return ionSequence.stream() + .map(IonCaseInsensitiveDecorator::wrapValue) + .toArray(IonValue[]::new); } @NotNull @Override @SuppressWarnings("unchecked") public T[] toArray(final T[] a) { - T[] rawArray = ionSequence.toArray(a); - int size = rawArray.length; - for (int i = 0; i < size; i++) { - rawArray[i] = (T) IonCaseInsensitiveDecorator.wrapValue((IonValue) rawArray[i]); - } - return rawArray; + Class type = a.getClass().getComponentType(); + return ionSequence.stream() + .map(IonCaseInsensitiveDecorator::wrapValue) + .map(i -> (T)i) + .toArray(size -> (T[]) Array.newInstance(type, size)); } - @Override @SuppressWarnings("unchecked") public T[] extract(final Class type) { - T[] rawArray = ionSequence.extract(type); - int size = rawArray.length; - for (int i = 0; i < size; i++) { - rawArray[i] = (T) IonCaseInsensitiveDecorator.wrapValue((IonValue) rawArray[i]); - } - return rawArray; + return ionSequence.stream() + .map(IonCaseInsensitiveDecorator::wrapValue) + .map(i -> (T)i) + .toArray(size -> (T[]) Array.newInstance(type, size)); } public IonSequence clone() throws UnknownSymbolException { diff --git a/serde/src/main/java/com/amazon/ionhiveserde/caseinsensitivedecorator/IonStructCaseInsensitiveDecorator.java b/serde/src/main/java/com/amazon/ionhiveserde/caseinsensitivedecorator/IonStructCaseInsensitiveDecorator.java index b463d4a..cd92ab1 100644 --- a/serde/src/main/java/com/amazon/ionhiveserde/caseinsensitivedecorator/IonStructCaseInsensitiveDecorator.java +++ b/serde/src/main/java/com/amazon/ionhiveserde/caseinsensitivedecorator/IonStructCaseInsensitiveDecorator.java @@ -88,6 +88,12 @@ public ValueFactory add(final String fieldName) { } @Override + /* + Argument `fieldName` is case sensitive so below method removes the desired value, which means it's possible that + struct.containsKey(fieldName) returns true while struct.remove(fieldName) does nothing. + + Refer to https://github.com/amzn/ion-hive-serde/issues/60. + */ public IonValue remove(final String fieldName) { return IonCaseInsensitiveDecorator.wrapValue(ionStruct.remove(fieldName)); } diff --git a/serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt b/serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt index d30a7c1..336029c 100644 --- a/serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt +++ b/serde/src/test/kotlin/com/amazon/ionhiveserde/configuration/CaseInsensitiveDecoratorTest.kt @@ -68,27 +68,30 @@ class CaseInsensitiveDecoratorTest { } @Test - fun ionStructCaseInsensitiveDecoratorGetRepeatedField() { - val struct = struct_for(" { Foo: 'bar', foo: 'Bar' }") - assertEquals(struct.containsKey("FOO"), true) - assertEquals(struct.get("Foo"), ION.newSymbol("bar")) - assertEquals(struct.get("foo"), ION.newSymbol("Bar")) + fun ionStructCaseInsensitiveDecoratorGetRepeatedFieldFound() { + val struct = struct_for(" { Foo: 'Bar', foo: 'bar' }") + assertEquals(struct.get("Foo"), ION.newSymbol("Bar")) + assertEquals(struct.get("foo"), ION.newSymbol("bar")) + } + + @Test + fun ionStructCaseInsensitiveDecoratorGetRepeatedFieldFoundIgnoringCase() { + val struct = struct_for(" { Foo: 'Bar', foo: 'bar' }") assert( - struct.get("FOO") == ION.newSymbol("Bar") || - struct.get("FOO") == ION.newSymbol("bar") + struct.get("FOO") == ION.newSymbol("Bar") || struct.get("FOO") == ION.newSymbol("bar") ) } @Test fun ionStructCaseInsensitiveDecoratorGetStruct() { val struct = struct_for(" { Foo: {} }") - assertTrue(struct.get("Foo") is IonStructCaseInsensitiveDecorator) + assertTrue(struct.get("Foo") is IonStructCaseInsensitiveDecorator, "Found ${struct.get("Foo").javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } @Test fun ionStructCaseInsensitiveDecoratorGetSequence() { val struct = struct_for(" { Foo: [] }") - assertTrue(struct.get("Foo") is IonSequenceCaseInsensitiveDecorator) + assertTrue(struct.get("Foo") is IonSequenceCaseInsensitiveDecorator, "Found ${struct.get("Foo").javaClass.simpleName} ,expected IonSequenceCaseInsensitiveDecorator type.") } @Test @@ -104,14 +107,14 @@ class CaseInsensitiveDecoratorTest { fun ionStructCaseInsensitiveDecoratorRemoveStruct() { val struct = struct_for(" { Foo: {} }") val s = struct.remove("Foo") - assertTrue(s is IonStructCaseInsensitiveDecorator) + assertTrue(s is IonStructCaseInsensitiveDecorator, "Found ${s.javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } @Test fun ionStructCaseInsensitiveDecoratorRemoveSequence() { val struct = struct_for(" { Foo: [] }") val s = struct.remove("Foo") - assertTrue(s is IonSequenceCaseInsensitiveDecorator) + assertTrue(s is IonSequenceCaseInsensitiveDecorator, "Found ${s.javaClass.simpleName} ,expected IonSequenceCaseInsensitiveDecorator type.") } @Test @@ -120,7 +123,7 @@ class CaseInsensitiveDecoratorTest { val s = struct.cloneAndRemove("Foo") assertEquals(s.size(), 0) - assertTrue(s is IonStructCaseInsensitiveDecorator) + assertTrue(s is IonStructCaseInsensitiveDecorator, "Found ${s.javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } @Test @@ -130,7 +133,7 @@ class CaseInsensitiveDecoratorTest { assertEquals(s.size(), 1) assertEquals(s.containsKey("foo"), true) - assertTrue(s is IonStructCaseInsensitiveDecorator) + assertTrue(s is IonStructCaseInsensitiveDecorator, "Found ${s.javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } @Test @@ -142,13 +145,13 @@ class CaseInsensitiveDecoratorTest { @Test fun ionSequenceCaseInsensitiveDecoratorGetStruct() { val sequence = sequence_for("[{}]") - assertTrue(sequence[0] is IonStructCaseInsensitiveDecorator) + assertTrue(sequence[0] is IonStructCaseInsensitiveDecorator, "Found ${sequence[0].javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } @Test fun ionSequenceCaseInsensitiveDecoratorGetSequence() { val sequence = sequence_for("[[]]") - assertTrue(sequence[0] is IonSequenceCaseInsensitiveDecorator) + assertTrue(sequence[0] is IonSequenceCaseInsensitiveDecorator, "Found ${sequence[0].javaClass.simpleName} ,expected IonSequenceCaseInsensitiveDecorator type.") } @Test @@ -164,14 +167,14 @@ class CaseInsensitiveDecoratorTest { fun ionSequenceCaseInsensitiveDecoratorSetStruct() { val sequence = sequence_for("[{}]") val l = sequence.set(0, ION.newInt(2)) - assertTrue(l is IonStructCaseInsensitiveDecorator) + assertTrue(l is IonStructCaseInsensitiveDecorator, "Found ${l.javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } @Test fun ionSequenceCaseInsensitiveDecoratorSetSequence() { val sequence = sequence_for("[[]]") val l = sequence.set(0, ION.newInt(2)) - assertTrue(l is IonSequenceCaseInsensitiveDecorator) + assertTrue(l is IonSequenceCaseInsensitiveDecorator, "Found ${l.javaClass.simpleName} ,expected IonSequenceCaseInsensitiveDecorator type.") } @Test @@ -188,7 +191,7 @@ class CaseInsensitiveDecoratorTest { val sequence = sequence_for("[{}]") val l = sequence.removeAt(0) - assertTrue(l is IonStructCaseInsensitiveDecorator) + assertTrue(l is IonStructCaseInsensitiveDecorator, "Found ${l.javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } @Test @@ -196,7 +199,7 @@ class CaseInsensitiveDecoratorTest { val sequence = sequence_for("[[]]") val l = sequence.removeAt(0) - assertTrue(l is IonSequenceCaseInsensitiveDecorator) + assertTrue(l is IonSequenceCaseInsensitiveDecorator, "Found ${l.javaClass.simpleName} ,expected IonSequenceCaseInsensitiveDecorator type.") } @Test @@ -204,8 +207,10 @@ class CaseInsensitiveDecoratorTest { val sequence = sequence_for("[1, [], {}]") val iter = sequence.listIterator() assertEquals(iter.next(), ION.newInt(1)) - assertTrue(iter.next() is IonSequenceCaseInsensitiveDecorator) - assertTrue(iter.next() is IonStructCaseInsensitiveDecorator) + val i = iter.next() + assertTrue(i is IonSequenceCaseInsensitiveDecorator, "Found ${i.javaClass.simpleName} ,expected IonSequenceCaseInsensitiveDecorator type.") + val ii = iter.next() + assertTrue(ii is IonStructCaseInsensitiveDecorator, "Found ${ii.javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } @Test @@ -214,7 +219,7 @@ class CaseInsensitiveDecoratorTest { val sublist = sequence.subList(0, 3) // sublist: [1, [], {}] assertEquals(sublist[0], ION.newInt(1)) - assertTrue(sublist[1] is IonSequenceCaseInsensitiveDecorator) - assertTrue(sublist[2] is IonStructCaseInsensitiveDecorator) + assertTrue(sublist[1] is IonSequenceCaseInsensitiveDecorator, "Found ${sublist[1].javaClass.simpleName} ,expected IonSequenceCaseInsensitiveDecorator type.") + assertTrue(sublist[2] is IonStructCaseInsensitiveDecorator, "Found ${sublist[2].javaClass.simpleName} ,expected IonStructCaseInsensitiveDecorator type.") } }