Repository navigation
Update Spark Parquet vectorized read tests to uses Iceberg Record instead of Avro GenericRecord #12925
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Spark Parquet vectorized read tests to uses Iceberg Record instead of Avro GenericRecord #12925
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,7 @@ | |
| import java.nio.ByteOrder; | ||
| import java.nio.file.Files; | ||
| import java.util.Arrays; | ||
| import java.util.Comparator; | ||
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
|
|
@@ -263,9 +264,16 @@ public void testRandomData() throws IOException { | |
|
|
||
| append.commit(); | ||
|
|
||
| Set<Record> records = Sets.newHashSet(IcebergGenerics.read(table).build()); | ||
| Comparator<Record> recordComparator = | ||
| Comparator.comparing((Record r) -> r.get(0, Long.class)) | ||
| .thenComparing( | ||
| (Record r) -> r.get(1, String.class), Comparator.nullsFirst(String::compareTo)); | ||
| List<Record> records = Lists.newArrayList(IcebergGenerics.read(table).build()); | ||
|
|
||
| expected.sort(recordComparator); | ||
| records.sort(recordComparator); | ||
|
Comment on lines
+267
to
+274
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See #12925 (comment) for why I changed this |
||
| assertThat(records).as("Should produce correct number of records").hasSameSizeAs(expected); | ||
| assertThat(records).as("Random record set should match").isEqualTo(Sets.newHashSet(expected)); | ||
| assertThat(records).as("Random record set should match").isEqualTo(expected); | ||
| } | ||
|
|
||
| @TestTemplate | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ | |
| import java.time.temporal.ChronoUnit; | ||
| import java.util.Collection; | ||
| import java.util.Date; | ||
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.UUID; | ||
|
|
@@ -49,6 +50,7 @@ | |
| import org.apache.spark.sql.catalyst.util.ArrayData; | ||
| import org.apache.spark.sql.catalyst.util.MapData; | ||
| import org.apache.spark.sql.types.Decimal; | ||
| import org.apache.spark.sql.vectorized.ColumnarBatch; | ||
| import org.apache.spark.unsafe.types.UTF8String; | ||
| import scala.collection.Seq; | ||
|
|
||
|
|
@@ -70,6 +72,15 @@ public static void assertEqualsSafe(Types.StructType struct, Record expected, Ro | |
| } | ||
| } | ||
|
|
||
| public static void assertEqualsBatch( | ||
| Types.StructType struct, Iterator<Record> expectedRecords, ColumnarBatch batch) { | ||
| for (int rowId = 0; rowId < batch.numRows(); rowId++) { | ||
| InternalRow row = batch.getRow(rowId); | ||
| Record expectedRecord = expectedRecords.next(); | ||
| assertEqualsUnsafe(struct, expectedRecord, row); | ||
| } | ||
| } | ||
|
|
||
| private static void assertEqualsSafe( | ||
| Types.ListType list, Collection<?> expected, List<?> actual) { | ||
| Type elementType = list.elementType(); | ||
|
|
@@ -289,11 +300,27 @@ private static void assertEqualsUnsafe(Type type, Object expected, Object actual | |
| } | ||
|
|
||
| switch (type.typeId()) { | ||
| case LONG: | ||
| assertThat(actual).as("Should be a long").isInstanceOf(Long.class); | ||
| if (expected instanceof Integer) { | ||
| assertThat(actual).as("Values didn't match").isEqualTo(((Number) expected).longValue()); | ||
| } else { | ||
| assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected); | ||
| } | ||
| break; | ||
| case DOUBLE: | ||
| assertThat(actual).as("Should be a double").isInstanceOf(Double.class); | ||
| if (expected instanceof Float) { | ||
| assertThat(Double.doubleToLongBits((double) actual)) | ||
| .as("Values didn't match") | ||
| .isEqualTo(Double.doubleToLongBits(((Number) expected).doubleValue())); | ||
| } else { | ||
| assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected); | ||
| } | ||
| break; | ||
|
Comment on lines
+303
to
+320
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is needed because of the int to long, float to double type promotion tests. This is the same as what exists in TestHelpers#assertEqualsUnsafe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should double check why this wasn't failing before.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Disregard, my first comment still stands. I confused myself, we only do |
||
| case BOOLEAN: | ||
| case INTEGER: | ||
| case LONG: | ||
| case FLOAT: | ||
| case DOUBLE: | ||
| assertThat(actual).as("Primitive value should be equal to expected").isEqualTo(expected); | ||
| break; | ||
| case DATE: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,9 +29,11 @@ | |
| import java.nio.file.Paths; | ||
| import java.util.Iterator; | ||
| import java.util.List; | ||
| import org.apache.avro.generic.GenericData; | ||
| import org.apache.iceberg.Files; | ||
| import org.apache.iceberg.Schema; | ||
| import org.apache.iceberg.data.RandomGenericData; | ||
| import org.apache.iceberg.data.Record; | ||
| import org.apache.iceberg.data.parquet.GenericParquetWriter; | ||
| import org.apache.iceberg.io.CloseableIterable; | ||
| import org.apache.iceberg.io.FileAppender; | ||
| import org.apache.iceberg.parquet.Parquet; | ||
|
|
@@ -40,7 +42,6 @@ | |
| import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.Iterables; | ||
| import org.apache.iceberg.spark.data.RandomData; | ||
| import org.apache.iceberg.spark.data.TestHelpers; | ||
| import org.apache.iceberg.spark.data.vectorized.VectorizedSparkParquetReaders; | ||
| import org.apache.iceberg.types.Types; | ||
|
|
@@ -71,14 +72,15 @@ public static void stopSpark() { | |
| } | ||
|
|
||
| @Override | ||
| Iterable<GenericData.Record> generateData( | ||
| Iterable<Record> generateData( | ||
| Schema schema, | ||
| int numRecords, | ||
| long seed, | ||
| float nullPercentage, | ||
| Function<GenericData.Record, GenericData.Record> transform) { | ||
| Function<Record, Record> transform) { | ||
| Iterable data = | ||
| RandomData.generateDictionaryEncodableData(schema, numRecords, seed, nullPercentage); | ||
| RandomGenericData.generateDictionaryEncodableRecords( | ||
| schema, numRecords, seed, nullPercentage); | ||
| return transform == IDENTITY ? data : Iterables.transform(data, transform); | ||
| } | ||
|
|
||
|
|
@@ -92,19 +94,16 @@ public void testMixedDictionaryNonDictionaryReads() throws IOException { | |
| Schema schema = new Schema(SUPPORTED_PRIMITIVES.fields()); | ||
| File dictionaryEncodedFile = File.createTempFile("junit", null, temp.toFile()); | ||
| assertThat(dictionaryEncodedFile.delete()).as("Delete should succeed").isTrue(); | ||
| Iterable<GenericData.Record> dictionaryEncodableData = | ||
| RandomData.generateDictionaryEncodableData( | ||
| schema, 10000, 0L, RandomData.DEFAULT_NULL_PERCENTAGE); | ||
| try (FileAppender<GenericData.Record> writer = | ||
| getParquetWriter(schema, dictionaryEncodedFile)) { | ||
| Iterable<Record> dictionaryEncodableData = | ||
| RandomGenericData.generateDictionaryEncodableRecords(schema, 10000, 0L); | ||
| try (FileAppender<Record> writer = getParquetWriter(schema, dictionaryEncodedFile)) { | ||
| writer.addAll(dictionaryEncodableData); | ||
| } | ||
|
|
||
| File plainEncodingFile = File.createTempFile("junit", null, temp.toFile()); | ||
| assertThat(plainEncodingFile.delete()).as("Delete should succeed").isTrue(); | ||
| Iterable<GenericData.Record> nonDictionaryData = | ||
| RandomData.generate(schema, 10000, 0L, RandomData.DEFAULT_NULL_PERCENTAGE); | ||
| try (FileAppender<GenericData.Record> writer = getParquetWriter(schema, plainEncodingFile)) { | ||
| Iterable<Record> nonDictionaryData = RandomGenericData.generate(schema, 10000, 0L); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code used to pass in an explicit null percentage of 5% as an explicit value to the generation function. Since 5% is already the default when it's not specified, I made the change to just call the utility without an explicit null percentage. |
||
| try (FileAppender<Record> writer = getParquetWriter(schema, plainEncodingFile)) { | ||
| writer.addAll(nonDictionaryData); | ||
| } | ||
|
|
||
|
|
@@ -132,12 +131,13 @@ public void testBinaryNotAllPagesDictionaryEncoded() throws IOException { | |
| File parquetFile = File.createTempFile("junit", null, temp.toFile()); | ||
| assertThat(parquetFile.delete()).as("Delete should succeed").isTrue(); | ||
|
|
||
| Iterable<GenericData.Record> records = RandomData.generateFallbackData(schema, 500, 0L, 100); | ||
| try (FileAppender<GenericData.Record> writer = | ||
| Iterable<Record> records = RandomGenericData.generateFallbackRecords(schema, 500, 0L, 100); | ||
| try (FileAppender<Record> writer = | ||
| Parquet.write(Files.localOutput(parquetFile)) | ||
| .schema(schema) | ||
| .set(PARQUET_DICT_SIZE_BYTES, "4096") | ||
| .set(PARQUET_PAGE_ROW_LIMIT, "100") | ||
| .createWriterFunc(GenericParquetWriter::create) | ||
| .build()) { | ||
| writer.addAll(records); | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.