RussellSpitzer commented on code in PR #13284:
URL: https://github.com/apache/iceberg/pull/13284#discussion_r2190637630
##########
api/src/test/java/org/apache/iceberg/variants/TestSerializedObject.java:
##########
@@ -182,70 +182,59 @@ public void testMixedValueTypes() {
assertThat(actualInner.get("f").asPrimitive().get()).isEqualTo((byte) 3);
}
- @Test
- public void testTwoByteOffsets() {
- // a string larger than 255 bytes to push the value offset size above 1
byte
- String randomString = RandomUtil.generateString(300, random);
+ @ParameterizedTest
+ @ValueSource(
+ ints = {
+ 300, // a big string larger than 255 bytes to push the value offset
size above 1 byte to
+ // test TwoByteOffsets
+ 70_000 // a really-big string larger than 65535 bytes to push the
value offset size above 1
+ // byte to test ThreeByteOffsets
+ })
+ public void testMultiByteOffsets(int multiByteOffset) {
+ String randomString = RandomUtil.generateString(multiByteOffset, random);
SerializedPrimitive bigString = VariantTestUtil.createString(randomString);
- // note that order doesn't matter. fields are sorted by name
- Map<String, VariantValue> data = ImmutableMap.of("big", bigString, "a",
I1, "b", I2, "c", I3);
- ByteBuffer meta = VariantTestUtil.createMetadata(data.keySet(), true /*
sort names */);
- ByteBuffer value = VariantTestUtil.createObject(meta, data);
-
- VariantMetadata metadata = VariantMetadata.from(meta);
- SerializedObject object = SerializedObject.from(metadata, value,
value.get(0));
-
- assertThat(object.type()).isEqualTo(PhysicalType.OBJECT);
- assertThat(object.numFields()).isEqualTo(4);
-
- assertThat(object.get("a").type()).isEqualTo(PhysicalType.INT8);
- assertThat(object.get("a").asPrimitive().get()).isEqualTo((byte) 1);
- assertThat(object.get("b").type()).isEqualTo(PhysicalType.INT8);
- assertThat(object.get("b").asPrimitive().get()).isEqualTo((byte) 2);
- assertThat(object.get("c").type()).isEqualTo(PhysicalType.INT8);
- assertThat(object.get("c").asPrimitive().get()).isEqualTo((byte) 3);
- assertThat(object.get("big").type()).isEqualTo(PhysicalType.STRING);
- assertThat(object.get("big").asPrimitive().get()).isEqualTo(randomString);
- }
-
- @Test
- public void testThreeByteOffsets() {
- // a string larger than 65535 bytes to push the value offset size above 1
byte
- String randomString = RandomUtil.generateString(70_000, random);
- SerializedPrimitive reallyBigString =
VariantTestUtil.createString(randomString);
-
// note that order doesn't matter. fields are sorted by name
Map<String, VariantValue> data =
- ImmutableMap.of("really-big", reallyBigString, "a", I1, "b", I2, "c",
I3);
+ ImmutableMap.of(
+ "small",
+ VariantTestUtil.createShortString("iceberg"),
+ "big",
+ bigString,
+ "a",
+ I1,
+ "b",
+ I2,
+ "c",
+ I3);
ByteBuffer meta = VariantTestUtil.createMetadata(data.keySet(), true /*
sort names */);
ByteBuffer value = VariantTestUtil.createObject(meta, data);
VariantMetadata metadata = VariantMetadata.from(meta);
SerializedObject object = SerializedObject.from(metadata, value,
value.get(0));
assertThat(object.type()).isEqualTo(PhysicalType.OBJECT);
- assertThat(object.numFields()).isEqualTo(4);
+ assertThat(object.numFields()).isEqualTo(5);
assertThat(object.get("a").type()).isEqualTo(PhysicalType.INT8);
assertThat(object.get("a").asPrimitive().get()).isEqualTo((byte) 1);
assertThat(object.get("b").type()).isEqualTo(PhysicalType.INT8);
assertThat(object.get("b").asPrimitive().get()).isEqualTo((byte) 2);
assertThat(object.get("c").type()).isEqualTo(PhysicalType.INT8);
assertThat(object.get("c").asPrimitive().get()).isEqualTo((byte) 3);
- assertThat(object.get("really-big").type()).isEqualTo(PhysicalType.STRING);
-
assertThat(object.get("really-big").asPrimitive().get()).isEqualTo(randomString);
+ VariantTestUtil.assertVariantString(object.get("big"), randomString);
+ VariantTestUtil.assertVariantString(object.get("small"), "iceberg");
}
@ParameterizedTest
@ValueSource(booleans = {true, false})
@SuppressWarnings({"unchecked", "rawtypes"})
public void testLargeObject(boolean sortFieldNames) {
- Map<String, SerializedPrimitive> fields = Maps.newHashMap();
+ Map<String, VariantPrimitive<?>> fields = Maps.newHashMap();
for (int i = 0; i < 10_000; i += 1) {
fields.put(
RandomUtil.generateString(10, random),
- VariantTestUtil.createString(RandomUtil.generateString(10, random)));
+ VariantTestUtil.createShortString(RandomUtil.generateString(10,
random)));
Review Comment:
Sorry for the delay, I think the change is fine here but it seems a little
weird to be using this test to check the short/big string serialization since I
think it's targeting just large objects. I would probably keep this test as is
and just change the
testLargeObjectUsingShortStringWithBigHeader
To a test that just covers
testShortStringsInVariantPrimitives
or something like that? I think the test code is basically the same,
although you maybe don't have to make such a large object, but that we can be
clear it isn't testing the "largeness" it's testing short strings in variant
primitives (as opposed to short strings
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
