From b5beef607bba3e0111b455cdf09ff09630f29236 Mon Sep 17 00:00:00 2001 From: Julien Le Dem Date: Wed, 22 Feb 2017 10:56:05 -0800 Subject: [PATCH 1/2] test case for union forward compat --- .../thrift/ThriftBytesWriteSupport.java | 6 +- .../thrift/TestUnionForwardCompat.java | 120 ++++++++++++++++++ parquet-thrift/src/test/thrift/compat.thrift | 26 ++++ pom.xml | 2 +- 4 files changed, 150 insertions(+), 4 deletions(-) create mode 100644 parquet-thrift/src/test/java/org/apache/parquet/thrift/TestUnionForwardCompat.java diff --git a/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftBytesWriteSupport.java b/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftBytesWriteSupport.java index f6f511b813..f45fab204e 100644 --- a/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftBytesWriteSupport.java +++ b/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftBytesWriteSupport.java @@ -140,9 +140,9 @@ private TProtocol protocol(BytesWritable record) { so if the data is corrupted, it could read a big integer as the length of the binary and therefore causes OOM to happen. Currently this fix only applies to TBinaryProtocol which has the setReadLength defined. */ - if (IS_READ_LENGTH_SETABLE && protocol instanceof TBinaryProtocol) { - ((TBinaryProtocol)protocol).setReadLength(record.getLength()); - } + //if (IS_READ_LENGTH_SETABLE && protocol instanceof TBinaryProtocol) { + // ((TBinaryProtocol)protocol).setReadLength(record.getLength()); + //} return protocol; } diff --git a/parquet-thrift/src/test/java/org/apache/parquet/thrift/TestUnionForwardCompat.java b/parquet-thrift/src/test/java/org/apache/parquet/thrift/TestUnionForwardCompat.java new file mode 100644 index 0000000000..c5076ac6d8 --- /dev/null +++ b/parquet-thrift/src/test/java/org/apache/parquet/thrift/TestUnionForwardCompat.java @@ -0,0 +1,120 @@ +/* + * 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.parquet.thrift; + +import java.io.ByteArrayInputStream; +import java.io.ByteArrayOutputStream; +import java.io.InputStream; +import java.io.OutputStream; + +import org.apache.parquet.thrift.test.compat.AString; +import org.apache.parquet.thrift.test.compat.EmptyStruct; +import org.apache.parquet.thrift.test.compat.TestForwardCompatRootV0; +import org.apache.parquet.thrift.test.compat.TestForwardCompatRootV1; +import org.apache.parquet.thrift.test.compat.TestForwardCompatRootV2; +import org.apache.parquet.thrift.test.compat.TestForwardCompatUnionV1; +import org.apache.parquet.thrift.test.compat.TestForwardCompatUnionV2; +import org.apache.thrift.TBase; +import org.apache.thrift.TException; +import org.apache.thrift.protocol.TCompactProtocol; +import org.apache.thrift.protocol.TProtocol; +import org.apache.thrift.transport.TIOStreamTransport; +import org.junit.Test; + +import junit.framework.Assert; + +public class TestUnionForwardCompat { + + + + @Test + public void testForward() throws TException, ReflectiveOperationException { + ByteArrayOutputStream out0 = new ByteArrayOutputStream(); + TestForwardCompatRootV0 v0 = new TestForwardCompatRootV0(); + v0.setStr("foo"); + v0.write(protocol(out0)); + + { + ByteArrayOutputStream out1 = new ByteArrayOutputStream(); + TestForwardCompatRootV1 v1 = read(TestForwardCompatRootV1.class, out0); + Assert.assertEquals(v0.getStr(), v1.getStr()); + Assert.assertNull(v1.getU()); + + v1.setU(TestForwardCompatUnionV1.empty1(new EmptyStruct())); + Assert.assertTrue(v1.getU().isSetEmpty1()); + v1.write(protocol(out1)); + + TestForwardCompatRootV2 v2 = read(TestForwardCompatRootV2.class, out1); + Assert.assertEquals(v1.getStr(), v2.getStr()); + Assert.assertTrue(v2.getU().isSetEmpty1()); + + } + + { + TestForwardCompatRootV2 v2 = new TestForwardCompatRootV2(); + v2.read(protocol(out0.toByteArray())); + Assert.assertEquals(v0.getStr(), v2.getStr()); + Assert.assertNull(v2.getU()); + } + } + + @Test + public void testBackwardV1ToV0() throws TException, ReflectiveOperationException { + ByteArrayOutputStream out1 = new ByteArrayOutputStream(); + TestForwardCompatRootV1 v1 = new TestForwardCompatRootV1(); + v1.setStr("foo"); + v1.setU(TestForwardCompatUnionV1.empty1(new EmptyStruct())); + v1.write(protocol(out1)); + + TestForwardCompatRootV0 v0 = read(TestForwardCompatRootV0.class, out1); + Assert.assertEquals(v0.getStr(), v1.getStr()); + } + + @Test + public void testBackwardV2ToV1() throws TException, ReflectiveOperationException { + ByteArrayOutputStream out2 = new ByteArrayOutputStream(); + TestForwardCompatRootV2 v2 = new TestForwardCompatRootV2(); + v2.setStr("foo"); + v2.setU(TestForwardCompatUnionV2.struct2(new AString("foo"))); + v2.write(protocol(out2)); + + TestForwardCompatRootV1 v1 = read(TestForwardCompatRootV1.class, out2); + Assert.assertEquals(v2.getStr(), v1.getStr()); + Assert.assertNull(v1.getU().getFieldValue()); + Assert.assertNull(v1.getU().getSetField()); + } + + private > T read(Class c, ByteArrayOutputStream buf) throws TException, ReflectiveOperationException { + T t = c.newInstance(); + t.read(protocol(buf.toByteArray())); + return t; + } + + private TProtocol protocol(byte[] byteArray) { + return protocol(new ByteArrayInputStream(byteArray)); + } + + private TCompactProtocol protocol(OutputStream to) { + return new TCompactProtocol(new TIOStreamTransport(to)); + } + + private TCompactProtocol protocol(InputStream from) { + return new TCompactProtocol(new TIOStreamTransport(from)); + } +} diff --git a/parquet-thrift/src/test/thrift/compat.thrift b/parquet-thrift/src/test/thrift/compat.thrift index 331657662c..b7365637d6 100644 --- a/parquet-thrift/src/test/thrift/compat.thrift +++ b/parquet-thrift/src/test/thrift/compat.thrift @@ -270,3 +270,29 @@ struct NestedEmptyStruct { 1: required EmptyStruct required_empty 2: optional EmptyStruct optional_empty } + +union TestForwardCompatUnionV1 { + 1: EmptyStruct empty1 + 2: string string2 +} + +union TestForwardCompatUnionV2 { + 1: EmptyStruct empty1 + 2: string string2_renamed + 3: AString struct2 +} + +struct TestForwardCompatRootV0 { + 1: required string str +} + +struct TestForwardCompatRootV1 { + 1: required string str + 2: optional TestForwardCompatUnionV1 u +} + +struct TestForwardCompatRootV2 { + 1: required string str + 2: optional TestForwardCompatUnionV2 u +} + diff --git a/pom.xml b/pom.xml index df4bbd3cb0..566c608aa4 100644 --- a/pom.xml +++ b/pom.xml @@ -82,7 +82,7 @@ false 0.16.0 h2 - 0.7.0 + 0.9.3 7.0.13 0.9.33 1.7.22 From cd5a8632940965769ef0b85a894540a02afe7419 Mon Sep 17 00:00:00 2001 From: Julien Le Dem Date: Thu, 2 Mar 2017 15:29:21 -0800 Subject: [PATCH 2/2] rollback version change --- .../parquet/hadoop/thrift/ThriftBytesWriteSupport.java | 6 +++--- pom.xml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftBytesWriteSupport.java b/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftBytesWriteSupport.java index f45fab204e..f6f511b813 100644 --- a/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftBytesWriteSupport.java +++ b/parquet-thrift/src/main/java/org/apache/parquet/hadoop/thrift/ThriftBytesWriteSupport.java @@ -140,9 +140,9 @@ private TProtocol protocol(BytesWritable record) { so if the data is corrupted, it could read a big integer as the length of the binary and therefore causes OOM to happen. Currently this fix only applies to TBinaryProtocol which has the setReadLength defined. */ - //if (IS_READ_LENGTH_SETABLE && protocol instanceof TBinaryProtocol) { - // ((TBinaryProtocol)protocol).setReadLength(record.getLength()); - //} + if (IS_READ_LENGTH_SETABLE && protocol instanceof TBinaryProtocol) { + ((TBinaryProtocol)protocol).setReadLength(record.getLength()); + } return protocol; } diff --git a/pom.xml b/pom.xml index 566c608aa4..df4bbd3cb0 100644 --- a/pom.xml +++ b/pom.xml @@ -82,7 +82,7 @@ false 0.16.0 h2 - 0.9.3 + 0.7.0 7.0.13 0.9.33 1.7.22