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 2021/12/20 21:48:02 UTC
[GitHub] [iceberg] kbendick opened a new pull request #3778: [SPARK][WIP] Fix NPE when casting partition columns to string type in getPartitions
kbendick opened a new pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778
When importing partitions into a table, we get all of the partitions using `Spark3Util#getPartitions`.
This converts the catalyst value to an Iceberg value, on which we finally call `toString`.
However, if one of the partition columns is `null`, this will fail and throw.
Presently, I'm just checking for `null` and then not calling `toString` on that, which makes the currently failing tests (after adding a `null` value into the rows) pass.
However, I'm going to add a dedicated test case for this vs sullying the current test cases.
--
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-unsubscribe@iceberg.apache.org
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 #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772756585
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -760,6 +809,19 @@ private void createCompositePartitionedTable(String format) {
compositePartitionedDF.write().insertInto(sourceTableName);
}
+ private void createCompositePartitionedTableWithNullValueInPartitionColumn(String format) {
+ String createParquet = "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING %s " +
+ "PARTITIONED BY (id, dept) LOCATION '%s'";
+ sql(createParquet, sourceTableName, format, fileTableDir.getAbsolutePath());
+
+ Dataset<Row> unionedDF = compositePartitionedDF.unionAll(compositePartitionedNullRecordDF)
+ .select("name", "subdept", "id", "dept")
+ .repartition(1);
+
+ unionedDF.write().insertInto(sourceTableName);
+ unionedDF.write().insertInto(sourceTableName);
Review comment:
Did you intend to write twice?
--
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-unsubscribe@iceberg.apache.org
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 #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772714325
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -401,6 +401,46 @@ public void addFilteredPartitionsToPartitioned2() {
sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(2L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue2() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('dept', 'hr'))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(6L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE dept = 'hr' ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
Review comment:
Instead of creating 2 new rather expensive tests, I can update the existing base `unpartitionedDF` to add the `null` record in there.
We'd have to update about 3 tests to get them passing though, as they assert on number of records total.
Let me know if we want to update existing tests or add new (but arguably expensive) tests.
--
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-unsubscribe@iceberg.apache.org
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 #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772756298
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -401,6 +401,46 @@ public void addFilteredPartitionsToPartitioned2() {
sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(2L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue2() {
Review comment:
2? Is there a better name to distinguish between this one and the test above?
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772860593
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -401,6 +401,46 @@ public void addFilteredPartitionsToPartitioned2() {
sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(2L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue2() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('dept', 'hr'))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(6L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE dept = 'hr' ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
Review comment:
You mean to say I should update the base `unpartitionedDF` and put a record with `null` in there and update the associated tests to account for that extra recrd?
It is a lot cleaner that way and less disruptive overall of the whole file's flow.
--
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-unsubscribe@iceberg.apache.org
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 #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#issuecomment-998937281
Thanks, @kbendick!
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772763659
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -401,6 +401,46 @@ public void addFilteredPartitionsToPartitioned2() {
sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(2L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue2() {
Review comment:
I updated the names of both tests.
There's `addFilteredPartitionsToPartitionedWithNullValueFilteringOnDept` and `addFilteredPartitionsToPartitionedWithNullValueFilteringOnId`.
Quite a mouthful, but it's better than just adding 2 at the end I suppose.
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772762517
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -401,6 +401,46 @@ public void addFilteredPartitionsToPartitioned2() {
sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(2L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue2() {
Review comment:
This is the naming format from `addFilteredPartitionsToPartitioned`, but I'll see about a better name.
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772762375
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -760,6 +809,19 @@ private void createCompositePartitionedTable(String format) {
compositePartitionedDF.write().insertInto(sourceTableName);
}
+ private void createCompositePartitionedTableWithNullValueInPartitionColumn(String format) {
+ String createParquet = "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING %s " +
+ "PARTITIONED BY (id, dept) LOCATION '%s'";
+ sql(createParquet, sourceTableName, format, fileTableDir.getAbsolutePath());
+
+ Dataset<Row> unionedDF = compositePartitionedDF.unionAll(compositePartitionedNullRecordDF)
+ .select("name", "subdept", "id", "dept")
+ .repartition(1);
+
+ unionedDF.write().insertInto(sourceTableName);
+ unionedDF.write().insertInto(sourceTableName);
Review comment:
Yes. All of the other tests wrote twice, so I kept it that way.
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK][WIP] Fix NPE when casting partition columns to string type in getPartitions
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772694186
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -777,7 +777,7 @@ public static TableIdentifier identifierToTableIdentifier(Identifier identifier)
int fieldIndex = schema.fieldIndex(field.name());
Object catalystValue = partition.values().get(fieldIndex, field.dataType());
Object value = CatalystTypeConverters.convertToScala(catalystValue, field.dataType());
- values.put(field.name(), value.toString());
+ values.put(field.name(), value == null ? null : value.toString());
Review comment:
Oh good call. I didn't like the in-line ternary here.
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK][WIP] Fix NPE when casting partition columns to string type in getPartitions
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772694841
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -777,7 +777,7 @@ public static TableIdentifier identifierToTableIdentifier(Identifier identifier)
int fieldIndex = schema.fieldIndex(field.name());
Object catalystValue = partition.values().get(fieldIndex, field.dataType());
Object value = CatalystTypeConverters.convertToScala(catalystValue, field.dataType());
- values.put(field.name(), value.toString());
+ values.put(field.name(), value == null ? null : value.toString());
Review comment:
`String.valueOf` also ensures that we get the string `"null"` as the value.
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on pull request #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
kbendick commented on pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#issuecomment-998352585
As this is a very simple fix, I'm adding it to the 0.13.0 release milestone.
--
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-unsubscribe@iceberg.apache.org
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 #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772756780
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -401,6 +401,46 @@ public void addFilteredPartitionsToPartitioned2() {
sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(2L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue2() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('dept', 'hr'))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(6L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE dept = 'hr' ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
Review comment:
I think it's worth this change.
--
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-unsubscribe@iceberg.apache.org
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 #3778: [SPARK][WIP] Fix NPE when casting partition columns to string type in getPartitions
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772692210
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -777,7 +777,7 @@ public static TableIdentifier identifierToTableIdentifier(Identifier identifier)
int fieldIndex = schema.fieldIndex(field.name());
Object catalystValue = partition.values().get(fieldIndex, field.dataType());
Object value = CatalystTypeConverters.convertToScala(catalystValue, field.dataType());
- values.put(field.name(), value.toString());
+ values.put(field.name(), value == null ? null : value.toString());
Review comment:
What about `String.valueOf(value)` to avoid the ternary expression?
--
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-unsubscribe@iceberg.apache.org
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 #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#issuecomment-998381030
Looks good to me other than the minor comments. Thanks, @kbendick!
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772860593
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -401,6 +401,46 @@ public void addFilteredPartitionsToPartitioned2() {
sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(2L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue2() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('dept', 'hr'))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(6L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE dept = 'hr' ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
Review comment:
You mean to say I should update the base `unpartitionedDF` and put a record with `null` in there and update the associated tests to account for that extra recrd?
It is a lot cleaner that way and less disruptive overall of the whole file.
--
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-unsubscribe@iceberg.apache.org
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] kbendick commented on a change in pull request #3778: [SPARK] Fix NPE when calling toString on partition column values in getPartitions (Spark 3.2)
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3778:
URL: https://github.com/apache/iceberg/pull/3778#discussion_r772860593
##########
File path: spark/v3.2/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java
##########
@@ -401,6 +401,46 @@ public void addFilteredPartitionsToPartitioned2() {
sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
}
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('id', 1))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(2L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE id = 1 ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
+ @Test
+ public void addFilteredPartitionsToPartitionedWithNullValue2() {
+ createCompositePartitionedTableWithNullValueInPartitionColumn("parquet");
+
+ String createIceberg =
+ "CREATE TABLE %s (id Integer, name String, dept String, subdept String) USING iceberg " +
+ "PARTITIONED BY (id, dept)";
+
+ sql(createIceberg, tableName);
+
+ Object result = scalarSql("CALL %s.system.add_files('%s', '`parquet`.`%s`', map('dept', 'hr'))",
+ catalogName, tableName, fileTableDir.getAbsolutePath());
+
+ Assert.assertEquals(6L, result);
+
+ assertEquals("Iceberg table contains correct data",
+ sql("SELECT id, name, dept, subdept FROM %s WHERE dept = 'hr' ORDER BY id", sourceTableName),
+ sql("SELECT id, name, dept, subdept FROM %s ORDER BY id", tableName));
+ }
+
Review comment:
You mean to say I should update the base `unpartitionedDF` and put a record with `null` in there and update the associated tests to account for that extra recrd?
It is _much_ cleaner that way.
--
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-unsubscribe@iceberg.apache.org
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