Skip to content

Commit

Permalink
Fixed review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@lschetanrao
lschetanrao committed Dec 8, 2023
1 parent bf24760 commit 10a0099
Show file tree
Hide file tree
Showing 16 changed files with 320 additions and 335 deletions.
73 changes: 34 additions & 39 deletions mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import static org.apache.iceberg.types.Types.NestedField.required;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.io.IOException;
import java.nio.file.Path;
Expand All @@ -40,7 +41,6 @@
import org.apache.iceberg.hadoop.HadoopTables;
import org.apache.iceberg.hive.HiveCatalog;
import org.apache.iceberg.types.Types;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;
Expand All @@ -53,7 +53,7 @@ public class TestCatalogs {

private Configuration conf;

@TempDir public Path temp;
@TempDir private Path temp;

@BeforeEach
public void before() {
Expand All @@ -64,7 +64,7 @@ public void before() {
public void testLoadTableFromLocation() throws IOException {
conf.set(CatalogUtil.ICEBERG_CATALOG_TYPE, Catalogs.LOCATION);

Assertions.assertThatThrownBy(() -> Catalogs.loadTable(conf))
assertThatThrownBy(() -> Catalogs.loadTable(conf))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Table location not set");

Expand All @@ -73,7 +73,7 @@ public void testLoadTableFromLocation() throws IOException {

conf.set(InputFormatConfig.TABLE_LOCATION, hadoopTable.location());

Assertions.assertThat(Catalogs.loadTable(conf).location()).isEqualTo(hadoopTable.location());
assertThat(Catalogs.loadTable(conf).location()).isEqualTo(hadoopTable.location());
}

@Test
Expand All @@ -82,7 +82,7 @@ public void testLoadTableFromCatalog() throws IOException {
String warehouseLocation = temp.resolve("hadoop").resolve("warehouse").toString();
setCustomCatalogProperties(defaultCatalogName, warehouseLocation);

Assertions.assertThatThrownBy(() -> Catalogs.loadTable(conf))
assertThatThrownBy(() -> Catalogs.loadTable(conf))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Table identifier not set");

Expand All @@ -91,24 +91,23 @@ public void testLoadTableFromCatalog() throws IOException {

conf.set(InputFormatConfig.TABLE_IDENTIFIER, "table");

Assertions.assertThat(Catalogs.loadTable(conf).location())
.isEqualTo(hadoopCatalogTable.location());
assertThat(Catalogs.loadTable(conf).location()).isEqualTo(hadoopCatalogTable.location());
}

@Test
public void testCreateDropTableToLocation() throws IOException {
Properties missingSchema = new Properties();
missingSchema.put("location", temp.resolve("hadoop_tables").toString());

Assertions.assertThatThrownBy(() -> Catalogs.createTable(conf, missingSchema))
assertThatThrownBy(() -> Catalogs.createTable(conf, missingSchema))
.isInstanceOf(NullPointerException.class)
.hasMessage("Table schema not set");

conf.set(CatalogUtil.ICEBERG_CATALOG_TYPE, Catalogs.LOCATION);
Properties missingLocation = new Properties();
missingLocation.put(InputFormatConfig.TABLE_SCHEMA, SchemaParser.toJson(SCHEMA));

Assertions.assertThatThrownBy(() -> Catalogs.createTable(conf, missingLocation))
assertThatThrownBy(() -> Catalogs.createTable(conf, missingLocation))
.isInstanceOf(NullPointerException.class)
.hasMessage("Table location not set");

Expand All @@ -123,22 +122,21 @@ public void testCreateDropTableToLocation() throws IOException {
HadoopTables tables = new HadoopTables();
Table table = tables.load(properties.getProperty("location"));

Assertions.assertThat(table.location()).isEqualTo(properties.getProperty("location"));
Assertions.assertThat(SchemaParser.toJson(table.schema()))
.isEqualTo(SchemaParser.toJson(SCHEMA));
Assertions.assertThat(PartitionSpecParser.toJson(table.spec()))
assertThat(table.location()).isEqualTo(properties.getProperty("location"));
assertThat(SchemaParser.toJson(table.schema())).isEqualTo(SchemaParser.toJson(SCHEMA));
assertThat(PartitionSpecParser.toJson(table.spec()))
.isEqualTo(PartitionSpecParser.toJson(SPEC));
assertThat(table.properties()).containsEntry("dummy", "test");

Assertions.assertThatThrownBy(() -> Catalogs.dropTable(conf, new Properties()))
assertThatThrownBy(() -> Catalogs.dropTable(conf, new Properties()))
.isInstanceOf(NullPointerException.class)
.hasMessage("Table location not set");

Properties dropProperties = new Properties();
dropProperties.put("location", temp.toFile() + "/hadoop_tables");
Catalogs.dropTable(conf, dropProperties);

Assertions.assertThatThrownBy(() -> Catalogs.loadTable(conf, dropProperties))
assertThatThrownBy(() -> Catalogs.loadTable(conf, dropProperties))
.isInstanceOf(NoSuchTableException.class)
.hasMessage("Table does not exist at location: " + properties.getProperty("location"));
}
Expand All @@ -155,14 +153,14 @@ public void testCreateDropTableToCatalog() throws IOException {
missingSchema.put("name", identifier.toString());
missingSchema.put(InputFormatConfig.CATALOG_NAME, defaultCatalogName);

Assertions.assertThatThrownBy(() -> Catalogs.createTable(conf, missingSchema))
assertThatThrownBy(() -> Catalogs.createTable(conf, missingSchema))
.isInstanceOf(NullPointerException.class)
.hasMessage("Table schema not set");

Properties missingIdentifier = new Properties();
missingIdentifier.put(InputFormatConfig.TABLE_SCHEMA, SchemaParser.toJson(SCHEMA));
missingIdentifier.put(InputFormatConfig.CATALOG_NAME, defaultCatalogName);
Assertions.assertThatThrownBy(() -> Catalogs.createTable(conf, missingIdentifier))
assertThatThrownBy(() -> Catalogs.createTable(conf, missingIdentifier))
.isInstanceOf(NullPointerException.class)
.hasMessage("Table identifier not set");

Expand All @@ -178,13 +176,12 @@ public void testCreateDropTableToCatalog() throws IOException {
HadoopCatalog catalog = new CustomHadoopCatalog(conf, warehouseLocation);
Table table = catalog.loadTable(identifier);

Assertions.assertThat(SchemaParser.toJson(table.schema()))
.isEqualTo(SchemaParser.toJson(SCHEMA));
Assertions.assertThat(PartitionSpecParser.toJson(table.spec()))
assertThat(SchemaParser.toJson(table.schema())).isEqualTo(SchemaParser.toJson(SCHEMA));
assertThat(PartitionSpecParser.toJson(table.spec()))
.isEqualTo(PartitionSpecParser.toJson(SPEC));
assertThat(table.properties()).containsEntry("dummy", "test");

Assertions.assertThatThrownBy(() -> Catalogs.dropTable(conf, new Properties()))
assertThatThrownBy(() -> Catalogs.dropTable(conf, new Properties()))
.isInstanceOf(NullPointerException.class)
.hasMessage("Table identifier not set");

Expand All @@ -193,7 +190,7 @@ public void testCreateDropTableToCatalog() throws IOException {
dropProperties.put(InputFormatConfig.CATALOG_NAME, defaultCatalogName);
Catalogs.dropTable(conf, dropProperties);

Assertions.assertThatThrownBy(() -> Catalogs.loadTable(conf, dropProperties))
assertThatThrownBy(() -> Catalogs.loadTable(conf, dropProperties))
.isInstanceOf(NoSuchTableException.class)
.hasMessage("Table does not exist: test.table");
}
Expand All @@ -202,11 +199,11 @@ public void testCreateDropTableToCatalog() throws IOException {
public void testLoadCatalogDefault() {
String catalogName = "barCatalog";
Optional<Catalog> defaultCatalog = Catalogs.loadCatalog(conf, catalogName);
Assertions.assertThat(defaultCatalog.isPresent()).isTrue();
Assertions.assertThat(defaultCatalog.get()).isInstanceOf(HiveCatalog.class);
assertThat(defaultCatalog).isPresent();
assertThat(defaultCatalog.get()).isInstanceOf(HiveCatalog.class);
Properties properties = new Properties();
properties.put(InputFormatConfig.CATALOG_NAME, catalogName);
Assertions.assertThat(Catalogs.hiveCatalog(conf, properties)).isTrue();
assertThat(Catalogs.hiveCatalog(conf, properties)).isTrue();
}

@Test
Expand All @@ -216,11 +213,11 @@ public void testLoadCatalogHive() {
InputFormatConfig.catalogPropertyConfigKey(catalogName, CatalogUtil.ICEBERG_CATALOG_TYPE),
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, catalogName);
Assertions.assertThat(hiveCatalog.isPresent()).isTrue();
Assertions.assertThat(hiveCatalog.get()).isInstanceOf(HiveCatalog.class);
assertThat(hiveCatalog).isPresent();
assertThat(hiveCatalog.get()).isInstanceOf(HiveCatalog.class);
Properties properties = new Properties();
properties.put(InputFormatConfig.CATALOG_NAME, catalogName);
Assertions.assertThat(Catalogs.hiveCatalog(conf, properties)).isTrue();
assertThat(Catalogs.hiveCatalog(conf, properties)).isTrue();
}

@Test
Expand All @@ -234,13 +231,13 @@ public void testLoadCatalogHadoop() {
catalogName, CatalogProperties.WAREHOUSE_LOCATION),
"/tmp/mylocation");
Optional<Catalog> hadoopCatalog = Catalogs.loadCatalog(conf, catalogName);
Assertions.assertThat(hadoopCatalog.isPresent()).isTrue();
Assertions.assertThat(hadoopCatalog.get()).isInstanceOf(HadoopCatalog.class);
Assertions.assertThat(hadoopCatalog.get().toString())
assertThat(hadoopCatalog).isPresent();
assertThat(hadoopCatalog.get()).isInstanceOf(HadoopCatalog.class);
assertThat(hadoopCatalog.get().toString())
.isEqualTo("HadoopCatalog{name=barCatalog, location=/tmp/mylocation}");
Properties properties = new Properties();
properties.put(InputFormatConfig.CATALOG_NAME, catalogName);
Assertions.assertThat(Catalogs.hiveCatalog(conf, properties)).isFalse();
assertThat(Catalogs.hiveCatalog(conf, properties)).isFalse();
}

@Test
Expand All @@ -254,18 +251,16 @@ public void testLoadCatalogCustom() {
catalogName, CatalogProperties.WAREHOUSE_LOCATION),
"/tmp/mylocation");
Optional<Catalog> customHadoopCatalog = Catalogs.loadCatalog(conf, catalogName);
Assertions.assertThat(customHadoopCatalog.isPresent()).isTrue();
Assertions.assertThat(customHadoopCatalog.get()).isInstanceOf(CustomHadoopCatalog.class);
assertThat(customHadoopCatalog).isPresent();
assertThat(customHadoopCatalog.get()).isInstanceOf(CustomHadoopCatalog.class);
Properties properties = new Properties();
properties.put(InputFormatConfig.CATALOG_NAME, catalogName);
Assertions.assertThat(Catalogs.hiveCatalog(conf, properties)).isFalse();
assertThat(Catalogs.hiveCatalog(conf, properties)).isFalse();
}

@Test
public void testLoadCatalogLocation() {
Assertions.assertThat(
Catalogs.loadCatalog(conf, Catalogs.ICEBERG_HADOOP_TABLE_NAME).isPresent())
.isFalse();
assertThat(Catalogs.loadCatalog(conf, Catalogs.ICEBERG_HADOOP_TABLE_NAME)).isNotPresent();
}

@Test
Expand All @@ -275,7 +270,7 @@ public void testLoadCatalogUnknown() {
InputFormatConfig.catalogPropertyConfigKey(catalogName, CatalogUtil.ICEBERG_CATALOG_TYPE),
"fooType");

Assertions.assertThatThrownBy(() -> Catalogs.loadCatalog(conf, catalogName))
assertThatThrownBy(() -> Catalogs.loadCatalog(conf, catalogName))
.isInstanceOf(UnsupportedOperationException.class)
.hasMessage("Unknown catalog type: fooType");
}
Expand Down
18 changes: 7 additions & 11 deletions mr/src/test/java/org/apache/iceberg/mr/hive/HiveIcebergTestUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package org.apache.iceberg.mr.hive;

import static org.apache.iceberg.types.Types.NestedField.optional;
import static org.assertj.core.api.Assertions.assertThat;

import java.io.File;
import java.io.IOException;
Expand Down Expand Up @@ -63,7 +64,6 @@
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.ByteBuffers;
import org.assertj.core.api.Assertions;

public class HiveIcebergTestUtils {
// TODO: Can this be a constant all around the Iceberg tests?
Expand Down Expand Up @@ -218,12 +218,10 @@ public static void assertEquals(Record expected, Record actual) {
for (int i = 0; i < expected.size(); ++i) {
if (expected.get(i) instanceof OffsetDateTime) {
// For OffsetDateTime we just compare the actual instant
Assertions.assertThat(((OffsetDateTime) actual.get(i)).toInstant())
assertThat(((OffsetDateTime) actual.get(i)).toInstant())
.isEqualTo(((OffsetDateTime) expected.get(i)).toInstant());
} else if (expected.get(i) instanceof byte[]) {
Assertions.assertThat((byte[]) actual.get(i)).isEqualTo((byte[]) expected.get(i));
} else {
Assertions.assertThat(actual.get(i)).isEqualTo(expected.get(i));
assertThat(actual.get(i)).isEqualTo(expected.get(i));
}
}
}
Expand Down Expand Up @@ -264,7 +262,7 @@ public static void validateData(List<Record> expected, List<Record> actual, int
sortedExpected.sort(Comparator.comparingLong(record -> (Long) record.get(sortBy)));
sortedActual.sort(Comparator.comparingLong(record -> (Long) record.get(sortBy)));

Assertions.assertThat(sortedActual.size()).isEqualTo(sortedExpected.size());
assertThat(sortedActual).hasSize(sortedExpected.size());
for (int i = 0; i < sortedExpected.size(); ++i) {
assertEquals(sortedExpected.get(i), sortedActual.get(i));
}
Expand All @@ -287,10 +285,8 @@ public static void validateFiles(Table table, Configuration conf, JobID jobId, i
.filter(path -> !path.getFileName().toString().startsWith("."))
.collect(Collectors.toList());

Assertions.assertThat(dataFiles.size()).isEqualTo(dataFileNum);
Assertions.assertThat(
new File(HiveIcebergOutputCommitter.generateJobLocation(table.location(), conf, jobId))
.exists())
.isFalse();
assertThat(dataFiles).hasSize(dataFileNum);
assertThat(new File(HiveIcebergOutputCommitter.generateJobLocation(table.location(), conf, jobId)))
.doesNotExist();
}
}
20 changes: 10 additions & 10 deletions mr/src/test/java/org/apache/iceberg/mr/hive/TestDeserializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
package org.apache.iceberg.mr.hive;

import static org.apache.iceberg.types.Types.NestedField.optional;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assumptions.assumeThat;

import java.util.Arrays;
import java.util.Collections;
Expand All @@ -35,8 +37,6 @@
import org.apache.iceberg.hive.HiveVersion;
import org.apache.iceberg.mr.hive.serde.objectinspector.IcebergObjectInspector;
import org.apache.iceberg.types.Types;
import org.assertj.core.api.Assertions;
import org.junit.jupiter.api.Assumptions;
import org.junit.jupiter.api.Test;

public class TestDeserializer {
Expand Down Expand Up @@ -74,7 +74,7 @@ public void testSchemaDeserialize() {

Record actual = deserializer.deserialize(new Object[] {new LongWritable(1L), new Text("Bob")});

Assertions.assertThat(actual).isEqualTo(expected);
assertThat(actual).isEqualTo(expected);
}

@Test
Expand All @@ -92,7 +92,7 @@ public void testStructDeserialize() {

Record actual = deserializer.deserialize(new Object[] {new LongWritable(1L), new Text("Bob")});

Assertions.assertThat(actual).isEqualTo(expected);
assertThat(actual).isEqualTo(expected);
}

@Test
Expand Down Expand Up @@ -127,7 +127,7 @@ public void testMapDeserialize() {
Object[] data = new Object[] {map};
Record actual = deserializer.deserialize(data);

Assertions.assertThat(actual).isEqualTo(expected);
assertThat(actual).isEqualTo(expected);
}

@Test
Expand Down Expand Up @@ -155,13 +155,13 @@ public void testListDeserialize() {
Object[] data = new Object[] {new Object[] {new LongWritable(1L)}};
Record actual = deserializer.deserialize(data);

Assertions.assertThat(actual).isEqualTo(expected);
assertThat(actual).isEqualTo(expected);
}

@Test
public void testDeserializeEverySupportedType() {
Assumptions.assumeFalse(
HiveVersion.min(HiveVersion.HIVE_3), "No test yet for Hive3 (Date/Timestamp creation)");
assumeThat(HiveVersion.min(HiveVersion.HIVE_3))
.as("No test yet for Hive3 (Date/Timestamp creation)");

Deserializer deserializer =
new Deserializer.Builder()
Expand Down Expand Up @@ -196,9 +196,9 @@ public void testNullDeserialize() {

Record actual = deserializer.deserialize(nulls);

Assertions.assertThat(actual).isEqualTo(expected);
assertThat(actual).isEqualTo(expected);

// Check null record as well
Assertions.assertThat(deserializer.deserialize(null)).isNull();
assertThat(deserializer.deserialize(null)).isNull();
}
}
Loading

0 comments on commit 10a0099

Please sign in to comment.