You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2020/12/14 23:53:39 UTC
[GitHub] [iceberg] karuppayya opened a new pull request #1932: Refactor Spark schema evolution tests
karuppayya opened a new pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] karuppayya commented on a change in pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
karuppayya commented on a change in pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#discussion_r543646141
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -120,13 +161,25 @@ public void renameColumn() {
results.createOrReplaceTempView("table");
spark.sql("select * from table").show();
+ List<String> fields = Arrays.asList(spark.sql("select * from table").schema().names());
+ assert (fields.contains("writer"));
+ assert (!fields.contains("author"));
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#discussion_r542942340
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -84,29 +98,56 @@ public void before() throws IOException {
@Test
public void addColumnToSchema() {
- table.updateSchema().addColumn("publisher", Types.StringType.get()).commit();
- Dataset<Row> df2 = spark.read().json(dataLocation + "new-books.json");
+ String fieldName = "publisher";
+ Schema schema = table.schema();
+ Assert.assertNull(schema.findField(fieldName));
+
+ table.updateSchema().addColumn(fieldName, Types.StringType.get()).commit();
+
+ Dataset<Row> df1 = spark.read()
+ .json(dataLocation + "books.json");
+ df1 = df1.withColumn(fieldName, new Column(Literal$.MODULE$.apply(null)))
+ .selectExpr("title", "price", "author", "cast(published as timestamp)", "genre", "publisher");
+ Dataset<Row> df2 = spark.read().json(dataLocation + "new-books.json")
+ .selectExpr("title", "price", "author", "cast(published as timestamp)", "genre", "publisher");
+ List<Row> expected = df1.union(df2)
+ .collectAsList();
+
+ // Append data
df2.select(df2.col("title"), df2.col("price").cast(DataTypes.IntegerType),
df2.col("author"), df2.col("published").cast(DataTypes.TimestampType),
df2.col("genre"), df2.col("publisher")).write()
.format("iceberg")
.mode("append")
.save(tableLocation.toString());
+
+ // Read iceberg table
+ Dataset<Row> iceberg = spark.read()
+ .format("iceberg")
+ .load(tableLocation.toString());
+
+ String error = QueryTest$.MODULE$.checkAnswer(iceberg, expected);
Review comment:
This shouldn't use Spark test internals. There are assertions in Iceberg to check rows.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#issuecomment-745568607
Thanks @karuppayya! I merged this. Hopefully tests are a bit faster now!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] karuppayya commented on pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
karuppayya commented on pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#issuecomment-745534201
I have removed the data correctness changes. After the removal, there are some minor refactoring(assertions, initialization etc). There are no other significant improvements. We can take the changes either in this PR or in a future PR relating to these tests. @rdblue @aokolnychyi @RussellSpitzer
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#discussion_r542944123
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -22,18 +22,27 @@
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Stream;
+
Review comment:
Style: no blocks of imports, just one list ordered alphabetically.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#discussion_r542945011
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -120,13 +161,25 @@ public void renameColumn() {
results.createOrReplaceTempView("table");
spark.sql("select * from table").show();
+ List<String> fields = Arrays.asList(spark.sql("select * from table").schema().names());
+ assert (fields.contains("writer"));
+ assert (!fields.contains("author"));
Review comment:
This are good assertions to have, but they should be JUnit assertions and not `assert`.
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -84,29 +98,56 @@ public void before() throws IOException {
@Test
public void addColumnToSchema() {
- table.updateSchema().addColumn("publisher", Types.StringType.get()).commit();
- Dataset<Row> df2 = spark.read().json(dataLocation + "new-books.json");
+ String fieldName = "publisher";
+ Schema schema = table.schema();
+ Assert.assertNull(schema.findField(fieldName));
+
+ table.updateSchema().addColumn(fieldName, Types.StringType.get()).commit();
+
+ Dataset<Row> df1 = spark.read()
+ .json(dataLocation + "books.json");
+ df1 = df1.withColumn(fieldName, new Column(Literal$.MODULE$.apply(null)))
+ .selectExpr("title", "price", "author", "cast(published as timestamp)", "genre", "publisher");
+ Dataset<Row> df2 = spark.read().json(dataLocation + "new-books.json")
+ .selectExpr("title", "price", "author", "cast(published as timestamp)", "genre", "publisher");
+ List<Row> expected = df1.union(df2)
+ .collectAsList();
+
+ // Append data
df2.select(df2.col("title"), df2.col("price").cast(DataTypes.IntegerType),
df2.col("author"), df2.col("published").cast(DataTypes.TimestampType),
df2.col("genre"), df2.col("publisher")).write()
.format("iceberg")
.mode("append")
.save(tableLocation.toString());
+
+ // Read iceberg table
+ Dataset<Row> iceberg = spark.read()
+ .format("iceberg")
+ .load(tableLocation.toString());
+
+ String error = QueryTest$.MODULE$.checkAnswer(iceberg, expected);
+ Assert.assertNull(error);
+
}
@Test
public void deleteColumnFromSchema() {
- table.updateSchema().deleteColumn("genre").commit();
+ List<Row> rows = spark.read()
+ .format("iceberg")
+ .load(tableLocation.toString())
+ .drop("genre").collectAsList();
+ table.updateSchema().deleteColumn("genre").commit();
table.refresh();
+
Dataset<Row> results = spark.read()
.format("iceberg")
.load(tableLocation.toString());
-
- results.createOrReplaceTempView("table");
- spark.sql("select * from table").show();
+ Assert.assertFalse(Arrays.asList(results.schema().names()).contains("genre"));
Review comment:
This is a good assertion to add.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] karuppayya commented on a change in pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
karuppayya commented on a change in pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#discussion_r543644843
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -84,29 +98,56 @@ public void before() throws IOException {
@Test
public void addColumnToSchema() {
- table.updateSchema().addColumn("publisher", Types.StringType.get()).commit();
- Dataset<Row> df2 = spark.read().json(dataLocation + "new-books.json");
+ String fieldName = "publisher";
+ Schema schema = table.schema();
+ Assert.assertNull(schema.findField(fieldName));
+
+ table.updateSchema().addColumn(fieldName, Types.StringType.get()).commit();
+
+ Dataset<Row> df1 = spark.read()
+ .json(dataLocation + "books.json");
+ df1 = df1.withColumn(fieldName, new Column(Literal$.MODULE$.apply(null)))
+ .selectExpr("title", "price", "author", "cast(published as timestamp)", "genre", "publisher");
Review comment:
@rdblue Thanks for inputs, I have removed the correctness related changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#discussion_r542944588
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -22,18 +22,27 @@
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Stream;
+
import org.apache.commons.io.FileUtils;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.hadoop.HadoopTables;
import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.Column;
import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.QueryTest$;
import org.apache.spark.sql.Row;
import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.expressions.Literal$;
import org.apache.spark.sql.types.DataTypes;
-import org.junit.Before;
-import org.junit.Test;
+import org.apache.spark.sql.types.LongType$;
+import org.apache.spark.sql.types.StructField;
+import org.junit.*;
Review comment:
Style: no wildcard imports because it isn't obvious where symbols are coming from.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] karuppayya commented on a change in pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
karuppayya commented on a change in pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#discussion_r543645927
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -22,18 +22,27 @@
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Stream;
+
Review comment:
done.
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -22,18 +22,27 @@
import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
+import java.util.stream.Stream;
+
import org.apache.commons.io.FileUtils;
import org.apache.iceberg.PartitionSpec;
import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.hadoop.HadoopTables;
import org.apache.iceberg.types.Types;
+import org.apache.spark.sql.Column;
import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.QueryTest$;
import org.apache.spark.sql.Row;
import org.apache.spark.sql.SparkSession;
+import org.apache.spark.sql.catalyst.expressions.Literal$;
import org.apache.spark.sql.types.DataTypes;
-import org.junit.Before;
-import org.junit.Test;
+import org.apache.spark.sql.types.LongType$;
+import org.apache.spark.sql.types.StructField;
+import org.junit.*;
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #1932: Refactor Spark schema evolution tests
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #1932:
URL: https://github.com/apache/iceberg/pull/1932#discussion_r542943454
##########
File path: spark2/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
##########
@@ -84,29 +98,56 @@ public void before() throws IOException {
@Test
public void addColumnToSchema() {
- table.updateSchema().addColumn("publisher", Types.StringType.get()).commit();
- Dataset<Row> df2 = spark.read().json(dataLocation + "new-books.json");
+ String fieldName = "publisher";
+ Schema schema = table.schema();
+ Assert.assertNull(schema.findField(fieldName));
+
+ table.updateSchema().addColumn(fieldName, Types.StringType.get()).commit();
+
+ Dataset<Row> df1 = spark.read()
+ .json(dataLocation + "books.json");
+ df1 = df1.withColumn(fieldName, new Column(Literal$.MODULE$.apply(null)))
+ .selectExpr("title", "price", "author", "cast(published as timestamp)", "genre", "publisher");
Review comment:
I don't think that this additional check is needed. This is an example, not really a correctness or unit test. So we want the example to be as simple as possible to be readable as an example of using the API. This only lives in tests so that it is kept up to date and we know it doesn't fail.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org