nastra commented on code in PR #7551:
URL: https://github.com/apache/iceberg/pull/7551#discussion_r1188643794
##########
core/src/main/java/org/apache/iceberg/PartitionsTable.java:
##########
@@ -71,6 +73,7 @@ public class PartitionsTable extends BaseMetadataTable {
"equality_delete_file_count",
Types.IntegerType.get(),
"Count of equality delete files"));
+ this.isUnpartitionedTable =
Partitioning.partitionType(table).fields().size() < 1;
Review Comment:
```suggestion
this.isUnpartitionedTable =
Partitioning.partitionType(table).fields().isEmpty();
```
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -805,6 +807,50 @@ public void testPartitionSpecEvolutionRemoval() {
}
}
+ @Test
+ public void testPartitionSpecEvolutionToUnpartitioned() throws IOException {
+ preparePartitionedTable();
+
+ // Remove partition field
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
+
+ DataFile data10 =
+ DataFiles.builder(table.spec())
+ .withPath("/path/to/data-10.parquet")
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+ table.newFastAppend().appendFile(data10).commit();
+
+ PartitionsTable partitionsTable = new PartitionsTable(table);
+ // must contain the partition column even the current spec is
non-partitioned.
Review Comment:
```suggestion
// must contain the partition column even when the current spec is
non-partitioned.
```
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -805,6 +807,50 @@ public void testPartitionSpecEvolutionRemoval() {
}
}
+ @Test
+ public void testPartitionSpecEvolutionToUnpartitioned() throws IOException {
+ preparePartitionedTable();
+
+ // Remove partition field
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
+
+ DataFile data10 =
+ DataFiles.builder(table.spec())
+ .withPath("/path/to/data-10.parquet")
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+ table.newFastAppend().appendFile(data10).commit();
+
+ PartitionsTable partitionsTable = new PartitionsTable(table);
+ // must contain the partition column even the current spec is
non-partitioned.
+
Assertions.assertThat(partitionsTable.schema().findField("partition")).isNotNull();
+
+ TableScan scanNoFilter = partitionsTable.newScan().select("partition");
+ try (CloseableIterable<ContentFile<?>> files =
+ PartitionsTable.planFiles((StaticTableScan) scanNoFilter)) {
+ if (formatVersion == 2) {
+ // four partitioned data files, four partitioned delete files and one
non-partitioned data
+ // file.
+ Assert.assertEquals(9, Iterators.size(files.iterator()));
+ } else {
+ // four partitioned data files and one non-partitioned data file.
+ Assert.assertEquals(5, Iterators.size(files.iterator()));
Review Comment:
```suggestion
Assertions.assertThat(files).hasSize(5);
```
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -805,6 +807,50 @@ public void testPartitionSpecEvolutionRemoval() {
}
}
+ @Test
+ public void testPartitionSpecEvolutionToUnpartitioned() throws IOException {
+ preparePartitionedTable();
+
+ // Remove partition field
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
+
+ DataFile data10 =
+ DataFiles.builder(table.spec())
+ .withPath("/path/to/data-10.parquet")
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+ table.newFastAppend().appendFile(data10).commit();
+
+ PartitionsTable partitionsTable = new PartitionsTable(table);
+ // must contain the partition column even the current spec is
non-partitioned.
+
Assertions.assertThat(partitionsTable.schema().findField("partition")).isNotNull();
+
+ TableScan scanNoFilter = partitionsTable.newScan().select("partition");
+ try (CloseableIterable<ContentFile<?>> files =
+ PartitionsTable.planFiles((StaticTableScan) scanNoFilter)) {
+ if (formatVersion == 2) {
+ // four partitioned data files, four partitioned delete files and one
non-partitioned data
+ // file.
+ Assert.assertEquals(9, Iterators.size(files.iterator()));
Review Comment:
```suggestion
Assertions.assertThat(files).hasSize(9);
```
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -805,6 +807,50 @@ public void testPartitionSpecEvolutionRemoval() {
}
}
+ @Test
+ public void testPartitionSpecEvolutionToUnpartitioned() throws IOException {
+ preparePartitionedTable();
+
+ // Remove partition field
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
+
+ DataFile data10 =
+ DataFiles.builder(table.spec())
+ .withPath("/path/to/data-10.parquet")
+ .withRecordCount(10)
+ .withFileSizeInBytes(10)
+ .build();
+ table.newFastAppend().appendFile(data10).commit();
+
+ PartitionsTable partitionsTable = new PartitionsTable(table);
+ // must contain the partition column even the current spec is
non-partitioned.
+
Assertions.assertThat(partitionsTable.schema().findField("partition")).isNotNull();
+
+ TableScan scanNoFilter = partitionsTable.newScan().select("partition");
+ try (CloseableIterable<ContentFile<?>> files =
+ PartitionsTable.planFiles((StaticTableScan) scanNoFilter)) {
+ if (formatVersion == 2) {
+ // four partitioned data files, four partitioned delete files and one
non-partitioned data
+ // file.
+ Assert.assertEquals(9, Iterators.size(files.iterator()));
+ } else {
+ // four partitioned data files and one non-partitioned data file.
+ Assert.assertEquals(5, Iterators.size(files.iterator()));
+ }
+
+ // check for null partition value.
+ Assert.assertTrue(
Review Comment:
```suggestion
Assertions.assertThat(StreamSupport.stream(files.spliterator(), false))
.anyMatch(
file -> {
StructLike partition = file.partition();
return Objects.equals(null, partition.get(0, Object.class));
});
```
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -805,6 +807,50 @@ public void testPartitionSpecEvolutionRemoval() {
}
}
+ @Test
+ public void testPartitionSpecEvolutionToUnpartitioned() throws IOException {
+ preparePartitionedTable();
+
+ // Remove partition field
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
+
+ DataFile data10 =
Review Comment:
nit: maybe just `dataFile`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
