ebyhr commented on code in PR #12456:
URL: https://github.com/apache/iceberg/pull/12456#discussion_r1980741814
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java:
##########
@@ -830,6 +830,44 @@ public void invalidDataImportPartitioned() {
.hasMessageContaining("that matches the partition columns");
}
+ @TestTemplate
+ public void partitionColumnCountMismatchInFilter() {
+ createPartitionedHiveTable();
+
+ createIcebergTable(
+ "id Integer, name String, dept String, subdept String", "PARTITIONED
BY (id)");
+ assertThatThrownBy(
+ () ->
+ scalarSql(
+ "CALL %s.system.add_files(table => '%s', source_table =>
'%s', partition_filter => map('id', '0','dept', '1'))",
Review Comment:
nti: We can omit parameter names (table, source_table, partition_filter).
Same for another test.
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAddFilesProcedure.java:
##########
@@ -830,6 +830,44 @@ public void invalidDataImportPartitioned() {
.hasMessageContaining("that matches the partition columns");
}
+ @TestTemplate
+ public void partitionColumnCountMismatchInFilter() {
+ createPartitionedHiveTable();
+
+ createIcebergTable(
+ "id Integer, name String, dept String, subdept String", "PARTITIONED
BY (id)");
+ assertThatThrownBy(
+ () ->
+ scalarSql(
+ "CALL %s.system.add_files(table => '%s', source_table =>
'%s', partition_filter => map('id', '0','dept', '1'))",
+ catalogName, tableName, sourceTableName))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageStartingWith("Cannot add data files to target table")
+ .hasMessageContaining(
+ "because that table is partitioned, but the number of columns in
the provided partition filter (2)"
+ + " is greater than the number of partitioned columns in table
(1)");
+ }
+
+ @TestTemplate
+ public void invalidPartitionColumnsInFilter() {
+ createPartitionedHiveTable();
+
+ String icebergTablePartitionNames = "id";
+ createIcebergTable(
+ "id Integer, name String, dept String, subdept String",
+ String.format("PARTITIONED BY (%s)", icebergTablePartitionNames));
+ assertThatThrownBy(
+ () ->
+ scalarSql(
+ "CALL %s.system.add_files(table => '%s', source_table =>
'%s', partition_filter => map('dept', '1'))",
+ catalogName, tableName, sourceTableName))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageStartingWith("Cannot add files to target table")
+ .hasMessageContaining(
+ "specified partition filter refers to columns that are not
partitioned: '[%s]'", "dept")
Review Comment:
There is no need to use a parameter. I would inline `dept`.
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkTableUtil.java:
##########
@@ -1169,7 +1169,7 @@ public static void validatePartitionFilter(
Preconditions.checkArgument(
unMatchedFilters.isEmpty(),
"Cannot add files to target table %s. %s is partitioned but the
specified partition filter "
- + "refers to columns that are not partitioned: '%s' . Valid
partition columns %s",
+ + "refers to columns that are not partitioned: '%s' . Valid
partition columns: '%s'",
Review Comment:
`'...'` style looks weird to me when it takes several column names. I would
use `[...]` or add only `:`.
--
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
