From 4fc1f4887ccf7d7c074b667c539b4896148edd20 Mon Sep 17 00:00:00 2001 From: Nick Dimiduk Date: Mon, 26 Aug 2013 00:00:36 -0700 Subject: [PATCH] HBASE-9283 Struct trailing null handling --- .../java/org/apache/hadoop/hbase/types/Struct.java | 8 +- .../apache/hadoop/hbase/types/StructIterator.java | 12 ++- .../org/apache/hadoop/hbase/types/TestStruct.java | 9 +-- .../hbase/types/TestStructNullExtension.java | 88 ++++++++++++++++++++++ 4 files changed, 105 insertions(+), 12 deletions(-) create mode 100644 hbase-common/src/test/java/org/apache/hadoop/hbase/types/TestStructNullExtension.java diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/types/Struct.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/types/Struct.java index 8c2c38d..580f6aa 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/types/Struct.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/types/Struct.java @@ -104,9 +104,9 @@ public class Struct implements DataType { @SuppressWarnings("unchecked") @Override public int encodedLength(Object[] val) { - assert fields.length == val.length; + assert fields.length >= val.length; int sum = 0; - for (int i = 0; i < fields.length; i++) + for (int i = 0; i < val.length; i++) sum += fields[i].encodedLength(val[i]); return sum; } @@ -155,9 +155,9 @@ public class Struct implements DataType { @SuppressWarnings("unchecked") @Override public int encode(PositionedByteRange dst, Object[] val) { - assert fields.length == val.length; + assert fields.length >= val.length; int written = 0; - for (int i = 0; i < fields.length; i++) { + for (int i = 0; i < val.length; i++) { written += fields[i].encode(dst, val[i]); } return written; diff --git a/hbase-common/src/main/java/org/apache/hadoop/hbase/types/StructIterator.java b/hbase-common/src/main/java/org/apache/hadoop/hbase/types/StructIterator.java index 12b4225..3af98b3 100644 --- a/hbase-common/src/main/java/org/apache/hadoop/hbase/types/StructIterator.java +++ b/hbase-common/src/main/java/org/apache/hadoop/hbase/types/StructIterator.java @@ -69,7 +69,9 @@ public class StructIterator implements Iterator { @Override public boolean hasNext() { - return idx < types.length; + // hasNext can return true when position == length in the case of a + // nullable field trailing a struct. + return idx < types.length && src.getPosition() <= src.getLength(); } @Override @@ -78,7 +80,9 @@ public class StructIterator implements Iterator { @Override public Object next() { if (!hasNext()) throw new NoSuchElementException(); - return types[idx++].decode(src); + DataType t = types[idx++]; + if (src.getPosition() == src.getLength() && t.isNullable()) return null; + return t.decode(src); } /** @@ -87,6 +91,8 @@ public class StructIterator implements Iterator { */ public int skip() { if (!hasNext()) throw new NoSuchElementException(); - return types[idx++].skip(src); + DataType t = types[idx++]; + if (src.getPosition() == src.getLength() && t.isNullable()) return 0; + return t.skip(src); } } diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/types/TestStruct.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/types/TestStruct.java index 1ef17cd..f679035 100644 --- a/hbase-common/src/test/java/org/apache/hadoop/hbase/types/TestStruct.java +++ b/hbase-common/src/test/java/org/apache/hadoop/hbase/types/TestStruct.java @@ -154,11 +154,10 @@ public class TestStruct { final transient String str; public Pojo2(Object... vals) { - byte[] empty = new byte[0]; - byteField1Asc = vals.length > 0 ? (byte[]) vals[0] : empty; - byteField2Dsc = vals.length > 1 ? (byte[]) vals[1] : empty; - stringFieldDsc = vals.length > 2 ? (String) vals[2] : ""; - byteField3Dsc = vals.length > 3 ? (byte[]) vals[3] : empty; + byteField1Asc = vals.length > 0 ? (byte[]) vals[0] : null; + byteField2Dsc = vals.length > 1 ? (byte[]) vals[1] : null; + stringFieldDsc = vals.length > 2 ? (String) vals[2] : null; + byteField3Dsc = vals.length > 3 ? (byte[]) vals[3] : null; str = new StringBuilder() .append("{ ") .append(Bytes.toStringBinary(byteField1Asc)).append(", ") diff --git a/hbase-common/src/test/java/org/apache/hadoop/hbase/types/TestStructNullExtension.java b/hbase-common/src/test/java/org/apache/hadoop/hbase/types/TestStructNullExtension.java new file mode 100644 index 0000000..d764683 --- /dev/null +++ b/hbase-common/src/test/java/org/apache/hadoop/hbase/types/TestStructNullExtension.java @@ -0,0 +1,88 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.hbase.types; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; + +import java.util.Arrays; + +import org.apache.hadoop.hbase.SmallTests; +import org.apache.hadoop.hbase.util.PositionedByteRange; +import org.apache.hadoop.hbase.util.SimplePositionedByteRange; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +@Category(SmallTests.class) +public class TestStructNullExtension { + + @Test + public void testNullExtension() { + // the following field members are used because they're all nullable + Struct shortest = new StructBuilder() + .add(OrderedInt32.ASCENDING) + .add(OrderedString.ASCENDING) + .toStruct(); + Struct extended = new StructBuilder() + .add(OrderedInt32.ASCENDING) + .add(OrderedString.ASCENDING) + // intentionally include a wrapped instance to test wrapper behavior. + .add(new TerminatedWrapper(OrderedString.ASCENDING, "/")) + .add(OrderedNumeric.ASCENDING) + .toStruct(); + PositionedByteRange buf1 = new SimplePositionedByteRange(10); + PositionedByteRange buf2 = new SimplePositionedByteRange(10); + + Object[] val1 = new Object[] { 1, "foo" }; + int expectedLen = shortest.encode(buf1, val1); + + // test iterator + buf1.setPosition(0); + StructIterator it = extended.iterator(buf1); + it.skip(); + it.skip(); + assertEquals("Position should be at end. Broken test.", buf1.getLength(), buf1.getPosition()); + assertEquals("Failed to skip null element with extended struct.", 0, it.skip()); + assertEquals("Failed to skip null element with extended struct.", 0, it.skip()); + + buf1.setPosition(0); + it = extended.iterator(buf1); + assertEquals(1, it.next()); + assertEquals("foo", it.next()); + assertEquals("Position should be at end. Broken test.", buf1.getLength(), buf1.getPosition()); + assertNull("Failed to skip null element with extended struct.", it.next()); + assertNull("Failed to skip null element with extended struct.", it.next()); + + // test Struct + buf1.setPosition(0); + assertArrayEquals("Simple struct decoding is broken.", val1, shortest.decode(buf1)); + + buf1.setPosition(0); + assertArrayEquals("Decoding short value with extended struct should append null elements.", + Arrays.copyOf(val1, 4), extended.decode(buf1)); + + buf1.setPosition(0); + assertEquals( + "Encoding a short value with extended struct should have same result as using short struct.", + expectedLen, extended.encode(buf2, val1)); + assertArrayEquals( + "Encoding a short value with extended struct should have same result as using short struct", + buf1.getBytes(), buf2.getBytes()); + } +} -- 1.8.3.4