You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tomtongue (via GitHub)" <gi...@apache.org> on 2024/03/01 16:02:02 UTC
[PR] [WIP] Migrate Read in Core to JUnit5 [iceberg]
tomtongue opened a new pull request, #9849:
URL: https://github.com/apache/iceberg/pull/9849
Migrate the following "Read" in iceberg-core to JUnit 5 and AssertJ style for https://github.com/apache/iceberg/issues/9085.
## Current Progress (same as in the issue above)
Read:
- [x] `MetadataTableScanTestBase` (extends `TableTestBase`)
- [ ] `TestMetadataTableScans` (WIP)
- [ ] `TestMetadataTableScansWithPartitionEvolution`
- [ ] `TestEntriesMetadataTable`
- [ ] `TestBatchScans`
- [ ] `TestIncrementalDataTableScan`
- [ ] `TestFindFiles`
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511206091
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -196,56 +193,66 @@ public void testRollbacks() {
transaction.commitTransaction();
// Go back to snapshot "E"
table.manageSnapshots().rollbackTo(5).commit();
- Assert.assertEquals(5, table.currentSnapshot().snapshotId());
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(5);
filesMatch(Lists.newArrayList("B", "D", "E"), appendsBetweenScan(1, 5));
filesMatch(Lists.newArrayList("B", "D", "E"), appendsAfterScan(1));
}
- @Test
+ @TestTemplate
public void testIgnoreResiduals() throws IOException {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
- TableScan scan1 = table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3);
+ IncrementalAppendScan scan1 =
+ table
+ .newIncrementalAppendScan()
+ .filter(Expressions.equal("id", 5))
+ .fromSnapshotExclusive(1)
+ .toSnapshot(3);
try (CloseableIterable<CombinedScanTask> tasks = scan1.planTasks()) {
- Assert.assertTrue(
- "Tasks should not be empty", com.google.common.collect.Iterables.size(tasks) > 0);
+ assertThat(tasks).as("Tasks should not be empty").hasSizeGreaterThan(0);
for (CombinedScanTask combinedScanTask : tasks) {
for (FileScanTask fileScanTask : combinedScanTask.files()) {
- Assert.assertNotEquals(
- "Residuals must be preserved", Expressions.alwaysTrue(), fileScanTask.residual());
+ assertThat(fileScanTask.residual())
+ .as("Residuals must be preserved")
+ .isNotEqualTo(Expressions.alwaysTrue());
}
}
}
- TableScan scan2 =
- table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3).ignoreResiduals();
+ IncrementalAppendScan scan2 =
+ table
+ .newIncrementalAppendScan()
+ .filter(Expressions.equal("id", 5))
+ .fromSnapshotExclusive(1)
+ .toSnapshot(3)
+ .ignoreResiduals();
try (CloseableIterable<CombinedScanTask> tasks = scan2.planTasks()) {
- Assert.assertTrue(
- "Tasks should not be empty", com.google.common.collect.Iterables.size(tasks) > 0);
+ assertThat(tasks).as("Tasks should not be empty").hasSizeGreaterThan(0);
for (CombinedScanTask combinedScanTask : tasks) {
for (FileScanTask fileScanTask : combinedScanTask.files()) {
- Assert.assertEquals(
- "Residuals must be ignored", Expressions.alwaysTrue(), fileScanTask.residual());
+ assertThat(fileScanTask.residual())
+ .as("Residuals must be ignored")
+ .isEqualTo(Expressions.alwaysTrue());
}
}
}
}
- @Test
- public void testPlanWithExecutor() throws IOException {
+ @TestTemplate
+ public void testPlanWithExecutor() {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
AtomicInteger planThreadsIndex = new AtomicInteger(0);
- TableScan scan =
+ IncrementalAppendScan scan =
table
- .newScan()
- .appendsAfter(1)
+ .newIncrementalAppendScan()
+ .fromSnapshotExclusive(1)
Review Comment:
why was this changed?
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511186311
##########
core/src/test/java/org/apache/iceberg/MetadataTableScanTestBase.java:
##########
@@ -89,18 +88,16 @@ protected void validatePartition(
CloseableIterable<ManifestEntry<? extends ContentFile<?>>> entries,
int position,
int partitionValue) {
- Assert.assertTrue(
- "File scan tasks do not include correct file",
- StreamSupport.stream(entries.spliterator(), false)
- .anyMatch(
- entry -> {
- StructLike partition = entry.file().partition();
- if (position >= partition.size()) {
- return false;
- }
-
- return Objects.equals(partitionValue, partition.get(position, Object.class));
- }));
+ assertThat(StreamSupport.stream(entries.spliterator(), false))
+ .as("File scan tasks do not include correct file")
+ .anyMatch(
+ entry -> {
+ StructLike partition = entry.file().partition();
+ if (position >= partition.size()) {
+ return false;
+ }
Review Comment:
nit: please add newline after }
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511185544
##########
core/src/test/java/org/apache/iceberg/MetadataTableScanTestBase.java:
##########
@@ -89,18 +88,16 @@ protected void validatePartition(
CloseableIterable<ManifestEntry<? extends ContentFile<?>>> entries,
int position,
int partitionValue) {
- Assert.assertTrue(
- "File scan tasks do not include correct file",
- StreamSupport.stream(entries.spliterator(), false)
- .anyMatch(
- entry -> {
- StructLike partition = entry.file().partition();
- if (position >= partition.size()) {
- return false;
- }
-
- return Objects.equals(partitionValue, partition.get(position, Object.class));
- }));
+ assertThat(StreamSupport.stream(entries.spliterator(), false))
Review Comment:
```suggestion
assertThat(entries)
```
I think this should also work
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1512208471
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -196,56 +193,66 @@ public void testRollbacks() {
transaction.commitTransaction();
// Go back to snapshot "E"
table.manageSnapshots().rollbackTo(5).commit();
- Assert.assertEquals(5, table.currentSnapshot().snapshotId());
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(5);
filesMatch(Lists.newArrayList("B", "D", "E"), appendsBetweenScan(1, 5));
filesMatch(Lists.newArrayList("B", "D", "E"), appendsAfterScan(1));
}
- @Test
+ @TestTemplate
public void testIgnoreResiduals() throws IOException {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
- TableScan scan1 = table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3);
+ IncrementalAppendScan scan1 =
+ table
+ .newIncrementalAppendScan()
+ .filter(Expressions.equal("id", 5))
+ .fromSnapshotExclusive(1)
+ .toSnapshot(3);
try (CloseableIterable<CombinedScanTask> tasks = scan1.planTasks()) {
- Assert.assertTrue(
- "Tasks should not be empty", com.google.common.collect.Iterables.size(tasks) > 0);
+ assertThat(tasks).as("Tasks should not be empty").hasSizeGreaterThan(0);
for (CombinedScanTask combinedScanTask : tasks) {
for (FileScanTask fileScanTask : combinedScanTask.files()) {
- Assert.assertNotEquals(
- "Residuals must be preserved", Expressions.alwaysTrue(), fileScanTask.residual());
+ assertThat(fileScanTask.residual())
+ .as("Residuals must be preserved")
+ .isNotEqualTo(Expressions.alwaysTrue());
}
}
}
- TableScan scan2 =
- table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3).ignoreResiduals();
+ IncrementalAppendScan scan2 =
+ table
+ .newIncrementalAppendScan()
Review Comment:
As I commented above, `table.newScan().appendsBetween` is deprecated. Revert this part to the previous one.
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -196,56 +193,66 @@ public void testRollbacks() {
transaction.commitTransaction();
// Go back to snapshot "E"
table.manageSnapshots().rollbackTo(5).commit();
- Assert.assertEquals(5, table.currentSnapshot().snapshotId());
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(5);
filesMatch(Lists.newArrayList("B", "D", "E"), appendsBetweenScan(1, 5));
filesMatch(Lists.newArrayList("B", "D", "E"), appendsAfterScan(1));
}
- @Test
+ @TestTemplate
public void testIgnoreResiduals() throws IOException {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
- TableScan scan1 = table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3);
+ IncrementalAppendScan scan1 =
+ table
+ .newIncrementalAppendScan()
+ .filter(Expressions.equal("id", 5))
+ .fromSnapshotExclusive(1)
+ .toSnapshot(3);
try (CloseableIterable<CombinedScanTask> tasks = scan1.planTasks()) {
- Assert.assertTrue(
- "Tasks should not be empty", com.google.common.collect.Iterables.size(tasks) > 0);
+ assertThat(tasks).as("Tasks should not be empty").hasSizeGreaterThan(0);
for (CombinedScanTask combinedScanTask : tasks) {
for (FileScanTask fileScanTask : combinedScanTask.files()) {
- Assert.assertNotEquals(
- "Residuals must be preserved", Expressions.alwaysTrue(), fileScanTask.residual());
+ assertThat(fileScanTask.residual())
+ .as("Residuals must be preserved")
+ .isNotEqualTo(Expressions.alwaysTrue());
}
}
}
- TableScan scan2 =
- table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3).ignoreResiduals();
+ IncrementalAppendScan scan2 =
+ table
+ .newIncrementalAppendScan()
+ .filter(Expressions.equal("id", 5))
+ .fromSnapshotExclusive(1)
+ .toSnapshot(3)
+ .ignoreResiduals();
try (CloseableIterable<CombinedScanTask> tasks = scan2.planTasks()) {
- Assert.assertTrue(
- "Tasks should not be empty", com.google.common.collect.Iterables.size(tasks) > 0);
+ assertThat(tasks).as("Tasks should not be empty").hasSizeGreaterThan(0);
for (CombinedScanTask combinedScanTask : tasks) {
for (FileScanTask fileScanTask : combinedScanTask.files()) {
- Assert.assertEquals(
- "Residuals must be ignored", Expressions.alwaysTrue(), fileScanTask.residual());
+ assertThat(fileScanTask.residual())
+ .as("Residuals must be ignored")
+ .isEqualTo(Expressions.alwaysTrue());
}
}
}
}
- @Test
- public void testPlanWithExecutor() throws IOException {
+ @TestTemplate
+ public void testPlanWithExecutor() {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
AtomicInteger planThreadsIndex = new AtomicInteger(0);
- TableScan scan =
+ IncrementalAppendScan scan =
table
- .newScan()
- .appendsAfter(1)
+ .newIncrementalAppendScan()
+ .fromSnapshotExclusive(1)
Review Comment:
As I commented above, `table.newScan().appendsBetween` is deprecated. Revert this part to the previous one.
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -196,56 +193,66 @@ public void testRollbacks() {
transaction.commitTransaction();
// Go back to snapshot "E"
table.manageSnapshots().rollbackTo(5).commit();
- Assert.assertEquals(5, table.currentSnapshot().snapshotId());
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(5);
filesMatch(Lists.newArrayList("B", "D", "E"), appendsBetweenScan(1, 5));
filesMatch(Lists.newArrayList("B", "D", "E"), appendsAfterScan(1));
}
- @Test
+ @TestTemplate
public void testIgnoreResiduals() throws IOException {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
- TableScan scan1 = table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3);
+ IncrementalAppendScan scan1 =
+ table
+ .newIncrementalAppendScan()
Review Comment:
`table.newScan().appendsBetween` is marked as "deprecated" in https://iceberg.apache.org/javadoc/latest/org/apache/iceberg/TableScan.html so that I changed here.
> Deprecated.
since 1.0.0, will be removed in 2.0.0; use [Table.newIncrementalAppendScan()](https://iceberg.apache.org/javadoc/latest/org/apache/iceberg/Table.html#newIncrementalAppendScan()) instead.
But maybe this should be another PR, or keep it this time. Let me revert it to the previous version.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511191340
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -745,6 +745,12 @@ void assertEquals(String context, Object expected, Object actual) {
assertThat(actual).as(context).isEqualTo(expected);
}
}
+
+ void assertHasSize(String context, int expected, List<?> actualList) {
Review Comment:
```suggestion
void assertHasSize(String context, List<?> list, int expectedSize) {
```
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1514762594
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -1357,27 +1344,27 @@ private void testPositionDeletesBaseTableFilter(boolean transactional) {
(StructLike)
constantsMap(posDeleteTask, partitionType).get(MetadataColumns.PARTITION_COLUMN_ID);
int taskPartition = taskPartitionStruct.get(0, Integer.class);
- Assert.assertEquals("Expected correct partition on task's file", 0, filePartition);
- Assert.assertEquals("Expected correct partition on task's column", 0, taskPartition);
-
- Assert.assertEquals(
- "Expected correct partition spec id on task", 0, posDeleteTask.file().specId());
- Assert.assertEquals(
- "Expected correct partition spec id on constant column",
- 0,
- constantsMap(posDeleteTask, partitionType).get(MetadataColumns.SPEC_ID.fieldId()));
-
- Assert.assertEquals(
- "Expected correct delete file on task", FILE_A_DELETES.path(), posDeleteTask.file().path());
- Assert.assertEquals(
- "Expected correct delete file on constant column",
- FILE_A_DELETES.path(),
- constantsMap(posDeleteTask, partitionType).get(MetadataColumns.FILE_PATH.fieldId()));
+ assertThat(filePartition).as("Expected correct partition on task's file").isEqualTo(0);
+ assertThat(taskPartition).as("Expected correct partition on task's column").isEqualTo(0);
+
+ assertThat(posDeleteTask.file().specId())
+ .as("Expected correct partition spec id on task")
+ .isEqualTo(0);
+ assertThat(constantsMap(posDeleteTask, partitionType).get(MetadataColumns.SPEC_ID.fieldId()))
Review Comment:
If there's a better idea, please let me know.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1512141524
##########
core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java:
##########
@@ -68,20 +63,19 @@ public void testEntriesTableScan() {
Schema expectedSchema =
TypeUtil.join(readSchema, MetricsUtil.readableMetricsSchema(table.schema(), readSchema));
- assertEquals(
- "A tableScan.select() should prune the schema",
- expectedSchema.asStruct(),
- scan.schema().asStruct());
+ assertThat(scan.schema().asStruct())
+ .as("A tableScan.select() should prune the schema")
+ .isEqualTo(expectedSchema.asStruct());
FileScanTask file = Iterables.getOnlyElement(scan.planFiles());
- Assert.assertEquals(
- "Data file should be the table's manifest",
- Iterables.getOnlyElement(table.currentSnapshot().allManifests(table.io())).path(),
- file.file().path());
- Assert.assertEquals("Should contain 2 data file records", 2, file.file().recordCount());
+ assertThat(file.file().path())
+ .as("Data file should be the table's manifest")
+ .isEqualTo(
+ Iterables.getOnlyElement(table.currentSnapshot().allManifests(table.io())).path());
Review Comment:
Sure, replace it with `get(0)`.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1514762022
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -1357,27 +1344,27 @@ private void testPositionDeletesBaseTableFilter(boolean transactional) {
(StructLike)
constantsMap(posDeleteTask, partitionType).get(MetadataColumns.PARTITION_COLUMN_ID);
int taskPartition = taskPartitionStruct.get(0, Integer.class);
- Assert.assertEquals("Expected correct partition on task's file", 0, filePartition);
- Assert.assertEquals("Expected correct partition on task's column", 0, taskPartition);
-
- Assert.assertEquals(
- "Expected correct partition spec id on task", 0, posDeleteTask.file().specId());
- Assert.assertEquals(
- "Expected correct partition spec id on constant column",
- 0,
- constantsMap(posDeleteTask, partitionType).get(MetadataColumns.SPEC_ID.fieldId()));
-
- Assert.assertEquals(
- "Expected correct delete file on task", FILE_A_DELETES.path(), posDeleteTask.file().path());
- Assert.assertEquals(
- "Expected correct delete file on constant column",
- FILE_A_DELETES.path(),
- constantsMap(posDeleteTask, partitionType).get(MetadataColumns.FILE_PATH.fieldId()));
+ assertThat(filePartition).as("Expected correct partition on task's file").isEqualTo(0);
+ assertThat(taskPartition).as("Expected correct partition on task's column").isEqualTo(0);
+
+ assertThat(posDeleteTask.file().specId())
+ .as("Expected correct partition spec id on task")
+ .isEqualTo(0);
+ assertThat(constantsMap(posDeleteTask, partitionType).get(MetadataColumns.SPEC_ID.fieldId()))
Review Comment:
Update the map tests with `.containsEntry()`. However each value is specified with the wildcard type like `capture of ?`, so the `contantsMap` value type needs to be changed in each test.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511200542
##########
core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java:
##########
@@ -146,22 +140,24 @@ public void testEntriesTableWithDeleteManifests() {
Schema expectedSchema =
TypeUtil.join(readSchema, MetricsUtil.readableMetricsSchema(table.schema(), readSchema));
- assertEquals(
- "A tableScan.select() should prune the schema",
- expectedSchema.asStruct(),
- scan.schema().asStruct());
+ assertThat(scan.schema().asStruct())
+ .as("A tableScan.select() should prune the schema")
+ .isEqualTo(expectedSchema.asStruct());
List<FileScanTask> files = ImmutableList.copyOf(scan.planFiles());
- Assert.assertEquals(
- "Data file should be the table's manifest",
- Iterables.getOnlyElement(table.currentSnapshot().dataManifests(table.io())).path(),
- files.get(0).file().path());
- Assert.assertEquals("Should contain 2 data file records", 2, files.get(0).file().recordCount());
- Assert.assertEquals(
- "Delete file should be in the table manifest",
- Iterables.getOnlyElement(table.currentSnapshot().deleteManifests(table.io())).path(),
- files.get(1).file().path());
- Assert.assertEquals(
- "Should contain 1 delete file record", 1, files.get(1).file().recordCount());
+ assertThat(files.get(0).file().path())
+ .as("Data file should be the table's manifest")
+ .isEqualTo(
+ Iterables.getOnlyElement(table.currentSnapshot().dataManifests(table.io())).path());
Review Comment:
same as above (and also applies to all the other places modified in this PR)
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511196427
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -745,6 +745,12 @@ void assertEquals(String context, Object expected, Object actual) {
assertThat(actual).as(context).isEqualTo(expected);
}
}
+
+ void assertHasSize(String context, int expected, List<?> actualList) {
Review Comment:
as I mentioned in another comment, I don't think we should introduce this method as part of this PR
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1512150409
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -31,49 +35,42 @@
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
-import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public class TestIncrementalDataTableScan extends TableTestBase {
- @Parameterized.Parameters(name = "formatVersion = {0}")
- public static Object[] parameters() {
- return new Object[] {1, 2};
- }
-
- public TestIncrementalDataTableScan(int formatVersion) {
- super(formatVersion);
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestIncrementalDataTableScan extends TestBase {
+ @Parameters(name = "formatVersion = {0}")
+ public static List<Object> parameters() {
Review Comment:
Okay sure, thanks for the advice.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1512205470
##########
core/src/test/java/org/apache/iceberg/TestBatchScans.java:
##########
@@ -51,27 +48,27 @@ public void testDataTableScan() {
BatchScan scan = table.newBatchScan();
List<ScanTask> tasks = planTasks(scan);
- Assert.assertEquals("Expected 2 tasks", 2, tasks.size());
+ assertThat(tasks).hasSize(2);
FileScanTask t1 = tasks.get(0).asFileScanTask();
- Assert.assertEquals("Task file must match", t1.file().path(), FILE_A.path());
- V1Assert.assertEquals("Task deletes size must match", 0, t1.deletes().size());
- V2Assert.assertEquals("Task deletes size must match", 1, t1.deletes().size());
+ assertThat(FILE_A.path()).as("Task file must match").isEqualTo(t1.file().path());
+ V1Assert.assertHasSize("Task deletes size must match", 0, t1.deletes());
+ V2Assert.assertHasSize("Task deletes size must match", 1, t1.deletes());
Review Comment:
Got it, thank you. I will create another PR for that migration. For now, keep the original one.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#issuecomment-1982188699
@nastra Thank you! Reflected the minor comment. If there's any part that I should change, please let me know.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1512205833
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -745,6 +745,12 @@ void assertEquals(String context, Object expected, Object actual) {
assertThat(actual).as(context).isEqualTo(expected);
}
}
+
+ void assertHasSize(String context, int expected, List<?> actualList) {
Review Comment:
Sure, thanks. Remove this method.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#issuecomment-1976599386
@nastra Complete the migration of the above classes. Could you review this when you have time?
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9849:
URL: https://github.com/apache/iceberg/pull/9849
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1514880737
##########
core/src/test/java/org/apache/iceberg/TestFindFiles.java:
##########
@@ -184,33 +181,33 @@ public void testCaseSensitivity() {
.withMetadataMatching(Expressions.startsWith("FILE_PATH", "/path/to/data-a"))
.collect();
- Assert.assertEquals(pathSet(FILE_A), pathSet(files));
+ assertThat(pathSet(files)).isEqualTo(pathSet(FILE_A));
}
- @Test
+ @TestTemplate
public void testIncludeColumnStats() {
table.newAppend().appendFile(FILE_WITH_STATS).commit();
Iterable<DataFile> files = FindFiles.in(table).includeColumnStats().collect();
final DataFile file = files.iterator().next();
- Assert.assertEquals(FILE_WITH_STATS.columnSizes(), file.columnSizes());
- Assert.assertEquals(FILE_WITH_STATS.valueCounts(), file.valueCounts());
- Assert.assertEquals(FILE_WITH_STATS.nullValueCounts(), file.nullValueCounts());
- Assert.assertEquals(FILE_WITH_STATS.nanValueCounts(), file.nanValueCounts());
- Assert.assertEquals(FILE_WITH_STATS.lowerBounds(), file.lowerBounds());
- Assert.assertEquals(FILE_WITH_STATS.upperBounds(), file.upperBounds());
+ assertThat(file.columnSizes()).isEqualTo(FILE_WITH_STATS.columnSizes());
+ assertThat(file.valueCounts()).isEqualTo(FILE_WITH_STATS.valueCounts());
+ assertThat(file.nullValueCounts()).isEqualTo(FILE_WITH_STATS.nullValueCounts());
+ assertThat(file.nanValueCounts()).isEqualTo(FILE_WITH_STATS.nanValueCounts());
+ assertThat(file.lowerBounds()).isEqualTo(FILE_WITH_STATS.lowerBounds());
+ assertThat(file.upperBounds()).isEqualTo(FILE_WITH_STATS.upperBounds());
}
- @Test
+ @TestTemplate
public void testNoSnapshot() {
// a table has no snapshot when it just gets created and no data is loaded yet
// if not handled properly, NPE will be thrown in collect()
Iterable<DataFile> files = FindFiles.in(table).collect();
// verify an empty collection of data file is returned
- Assert.assertEquals(0, Sets.newHashSet(files).size());
+ assertThat(Sets.newHashSet(files)).hasSize(0);
Review Comment:
```suggestion
assertThat(files).hasSize(0);
```
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511187801
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -745,6 +745,12 @@ void assertEquals(String context, Object expected, Object actual) {
assertThat(actual).as(context).isEqualTo(expected);
}
}
+
+ void assertHasSize(String context, int expected, List<?> actualList) {
Review Comment:
I don't think we need this method. It's actually clearer to read the code when using `assertThat()` directly and this also allows to combine this with other checks
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511199403
##########
core/src/test/java/org/apache/iceberg/TestEntriesMetadataTable.java:
##########
@@ -68,20 +63,19 @@ public void testEntriesTableScan() {
Schema expectedSchema =
TypeUtil.join(readSchema, MetricsUtil.readableMetricsSchema(table.schema(), readSchema));
- assertEquals(
- "A tableScan.select() should prune the schema",
- expectedSchema.asStruct(),
- scan.schema().asStruct());
+ assertThat(scan.schema().asStruct())
+ .as("A tableScan.select() should prune the schema")
+ .isEqualTo(expectedSchema.asStruct());
FileScanTask file = Iterables.getOnlyElement(scan.planFiles());
- Assert.assertEquals(
- "Data file should be the table's manifest",
- Iterables.getOnlyElement(table.currentSnapshot().allManifests(table.io())).path(),
- file.file().path());
- Assert.assertEquals("Should contain 2 data file records", 2, file.file().recordCount());
+ assertThat(file.file().path())
+ .as("Data file should be the table's manifest")
+ .isEqualTo(
+ Iterables.getOnlyElement(table.currentSnapshot().allManifests(table.io())).path());
Review Comment:
can we express this check without using `Iterables`?
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511212604
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -356,14 +353,14 @@ public void testPartitionsTableScanNoFilter() {
Types.StructType.of(optional(1000, "data_bucket", Types.IntegerType.get()))))
.asStruct();
TableScan scanNoFilter = partitionsTable.newScan().select("partition.data_bucket");
- Assert.assertEquals(expected, scanNoFilter.schema().asStruct());
+ assertThat(scanNoFilter.schema().asStruct()).isEqualTo(expected);
CloseableIterable<ManifestEntry<?>> entries =
PartitionsTable.planEntries((StaticTableScan) scanNoFilter);
if (formatVersion == 2) {
- Assert.assertEquals(8, Iterators.size(entries.iterator()));
+ assertThat(entries.iterator()).toIterable().hasSize(8);
Review Comment:
can't we just use `assertThat(entries).hasSize(xx)` 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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511226566
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScansWithPartitionEvolution.java:
##########
@@ -239,15 +234,15 @@ public void testPartitionSpecEvolutionToUnpartitioned() throws IOException {
PartitionsTable partitionsTable = new PartitionsTable(table);
// must contain the partition column even when the current spec is non-partitioned.
- Assertions.assertThat(partitionsTable.schema().findField("partition")).isNotNull();
+ assertThat(partitionsTable.schema().findField("partition")).isNotNull();
try (CloseableIterable<ManifestEntry<?>> entries =
PartitionsTable.planEntries((StaticTableScan) partitionsTable.newScan())) {
// four partitioned data files and one non-partitioned data file.
- Assertions.assertThat(entries).hasSize(5);
+ assertThat(entries).hasSize(5);
// check for null partition value.
- Assertions.assertThat(StreamSupport.stream(entries.spliterator(), false))
+ assertThat(StreamSupport.stream(entries.spliterator(), false))
Review Comment:
can this be done without using `StreamSupport`?
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511226028
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScansWithPartitionEvolution.java:
##########
@@ -193,38 +190,36 @@ public void testPositionDeletesPartitionSpecRemoval() {
PositionDeletesScanTask posDeleteTask = (PositionDeletesScanTask) task;
int filePartition = posDeleteTask.file().partition().get(0, Integer.class);
- Assert.assertEquals("Expected correct partition on task", 1, filePartition);
+ assertThat(filePartition).as("Expected correct partition on task").isEqualTo(1);
// Constant partition struct is common struct that includes even deleted partition column
int taskConstantPartition =
((StructLike)
constantsMap(posDeleteTask, partitionType).get(MetadataColumns.PARTITION_COLUMN_ID))
.get(1, Integer.class);
- Assert.assertEquals("Expected correct partition on constant column", 1, taskConstantPartition);
-
- Assert.assertEquals(
- "Expected correct partition field id on task's spec",
- table.ops().current().spec().partitionType().fields().get(0).fieldId(),
- posDeleteTask.spec().fields().get(0).fieldId());
-
- Assert.assertEquals(
- "Expected correct partition spec id on task",
- table.ops().current().spec().specId(),
- posDeleteTask.file().specId());
- Assert.assertEquals(
- "Expected correct partition spec id on constant column",
- table.ops().current().spec().specId(),
- constantsMap(posDeleteTask, partitionType).get(MetadataColumns.SPEC_ID.fieldId()));
-
- Assert.assertEquals(
- "Expected correct delete file on task", deleteFile.path(), posDeleteTask.file().path());
- Assert.assertEquals(
- "Expected correct delete file on constant column",
- deleteFile.path(),
- constantsMap(posDeleteTask, partitionType).get(MetadataColumns.FILE_PATH.fieldId()));
+ assertThat(taskConstantPartition)
+ .as("Expected correct partition on constant column")
+ .isEqualTo(1);
+
+ assertThat(posDeleteTask.spec().fields().get(0).fieldId())
+ .as("Expected correct partition field id on task's spec")
+ .isEqualTo(table.ops().current().spec().partitionType().fields().get(0).fieldId());
+
+ assertThat(posDeleteTask.file().specId())
+ .as("Expected correct partition spec id on task")
+ .isEqualTo(table.ops().current().spec().specId());
+ assertThat(constantsMap(posDeleteTask, partitionType).get(MetadataColumns.SPEC_ID.fieldId()))
Review Comment:
this should use .containsEntry()
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511187801
##########
core/src/test/java/org/apache/iceberg/TestBase.java:
##########
@@ -745,6 +745,12 @@ void assertEquals(String context, Object expected, Object actual) {
assertThat(actual).as(context).isEqualTo(expected);
}
}
+
+ void assertHasSize(String context, int expected, List<?> actualList) {
Review Comment:
I don't think we need this method. It's actually clearer to read the code when using `assertThat()` directly and this also allows to combine this with other checks
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511205551
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -196,56 +193,66 @@ public void testRollbacks() {
transaction.commitTransaction();
// Go back to snapshot "E"
table.manageSnapshots().rollbackTo(5).commit();
- Assert.assertEquals(5, table.currentSnapshot().snapshotId());
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(5);
filesMatch(Lists.newArrayList("B", "D", "E"), appendsBetweenScan(1, 5));
filesMatch(Lists.newArrayList("B", "D", "E"), appendsAfterScan(1));
}
- @Test
+ @TestTemplate
public void testIgnoreResiduals() throws IOException {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
- TableScan scan1 = table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3);
+ IncrementalAppendScan scan1 =
+ table
+ .newIncrementalAppendScan()
+ .filter(Expressions.equal("id", 5))
+ .fromSnapshotExclusive(1)
+ .toSnapshot(3);
try (CloseableIterable<CombinedScanTask> tasks = scan1.planTasks()) {
- Assert.assertTrue(
- "Tasks should not be empty", com.google.common.collect.Iterables.size(tasks) > 0);
+ assertThat(tasks).as("Tasks should not be empty").hasSizeGreaterThan(0);
for (CombinedScanTask combinedScanTask : tasks) {
for (FileScanTask fileScanTask : combinedScanTask.files()) {
- Assert.assertNotEquals(
- "Residuals must be preserved", Expressions.alwaysTrue(), fileScanTask.residual());
+ assertThat(fileScanTask.residual())
+ .as("Residuals must be preserved")
+ .isNotEqualTo(Expressions.alwaysTrue());
}
}
}
- TableScan scan2 =
- table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3).ignoreResiduals();
+ IncrementalAppendScan scan2 =
+ table
+ .newIncrementalAppendScan()
Review Comment:
why has this changed?
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511203446
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -31,49 +35,42 @@
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
-import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public class TestIncrementalDataTableScan extends TableTestBase {
- @Parameterized.Parameters(name = "formatVersion = {0}")
- public static Object[] parameters() {
- return new Object[] {1, 2};
- }
-
- public TestIncrementalDataTableScan(int formatVersion) {
- super(formatVersion);
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestIncrementalDataTableScan extends TestBase {
+ @Parameters(name = "formatVersion = {0}")
+ public static List<Object> parameters() {
Review Comment:
```suggestion
protected static List<Object> parameters() {
```
this was most likely previously required to be public by JUnit4, but that's not the case anymore with JUnit5, so we can make all of the parameters() methods protected
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511221874
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -382,13 +379,13 @@ public void testPartitionsTableScanWithProjection() {
.asStruct();
TableScan scanWithProjection = partitionsTable.newScan().select("file_count");
- Assert.assertEquals(expected, scanWithProjection.schema().asStruct());
+ assertThat(scanWithProjection.schema().asStruct()).isEqualTo(expected);
CloseableIterable<ManifestEntry<?>> entries =
PartitionsTable.planEntries((StaticTableScan) scanWithProjection);
if (formatVersion == 2) {
- Assert.assertEquals(8, Iterators.size(entries.iterator()));
+ assertThat(entries.iterator()).toIterable().hasSize(8);
Review Comment:
same as 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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511195274
##########
core/src/test/java/org/apache/iceberg/TestBatchScans.java:
##########
@@ -51,27 +48,27 @@ public void testDataTableScan() {
BatchScan scan = table.newBatchScan();
List<ScanTask> tasks = planTasks(scan);
- Assert.assertEquals("Expected 2 tasks", 2, tasks.size());
+ assertThat(tasks).hasSize(2);
FileScanTask t1 = tasks.get(0).asFileScanTask();
- Assert.assertEquals("Task file must match", t1.file().path(), FILE_A.path());
- V1Assert.assertEquals("Task deletes size must match", 0, t1.deletes().size());
- V2Assert.assertEquals("Task deletes size must match", 1, t1.deletes().size());
+ assertThat(FILE_A.path()).as("Task file must match").isEqualTo(t1.file().path());
+ V1Assert.assertHasSize("Task deletes size must match", 0, t1.deletes());
+ V2Assert.assertHasSize("Task deletes size must match", 1, t1.deletes());
Review Comment:
I think changing this should be done separately from migrating away from JUnit4. That means that we can consider making `TableAssertions` more fluent and more like AssertJ assertions, but that shouldn't be part of this PR
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511201452
##########
core/src/test/java/org/apache/iceberg/TestFindFiles.java:
##########
@@ -18,40 +18,37 @@
*/
package org.apache.iceberg;
+import static org.assertj.core.api.Assertions.assertThat;
+
import java.util.Arrays;
+import java.util.List;
import java.util.Set;
import org.apache.iceberg.expressions.Expressions;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Conversions;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public class TestFindFiles extends TableTestBase {
- @Parameterized.Parameters(name = "formatVersion = {0}")
- public static Object[] parameters() {
- return new Object[] {1, 2};
- }
-
- public TestFindFiles(int formatVersion) {
- super(formatVersion);
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestFindFiles extends TestBase {
+ @Parameters(name = "formatVersion = {0}")
+ public static List<Object> parameters() {
Review Comment:
```suggestion
protected static List<Object> parameters() {
```
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511205033
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -196,56 +193,66 @@ public void testRollbacks() {
transaction.commitTransaction();
// Go back to snapshot "E"
table.manageSnapshots().rollbackTo(5).commit();
- Assert.assertEquals(5, table.currentSnapshot().snapshotId());
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(5);
filesMatch(Lists.newArrayList("B", "D", "E"), appendsBetweenScan(1, 5));
filesMatch(Lists.newArrayList("B", "D", "E"), appendsAfterScan(1));
}
- @Test
+ @TestTemplate
public void testIgnoreResiduals() throws IOException {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
- TableScan scan1 = table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3);
+ IncrementalAppendScan scan1 =
+ table
+ .newIncrementalAppendScan()
Review Comment:
why has this been changed?
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1511224807
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -1357,27 +1344,27 @@ private void testPositionDeletesBaseTableFilter(boolean transactional) {
(StructLike)
constantsMap(posDeleteTask, partitionType).get(MetadataColumns.PARTITION_COLUMN_ID);
int taskPartition = taskPartitionStruct.get(0, Integer.class);
- Assert.assertEquals("Expected correct partition on task's file", 0, filePartition);
- Assert.assertEquals("Expected correct partition on task's column", 0, taskPartition);
-
- Assert.assertEquals(
- "Expected correct partition spec id on task", 0, posDeleteTask.file().specId());
- Assert.assertEquals(
- "Expected correct partition spec id on constant column",
- 0,
- constantsMap(posDeleteTask, partitionType).get(MetadataColumns.SPEC_ID.fieldId()));
-
- Assert.assertEquals(
- "Expected correct delete file on task", FILE_A_DELETES.path(), posDeleteTask.file().path());
- Assert.assertEquals(
- "Expected correct delete file on constant column",
- FILE_A_DELETES.path(),
- constantsMap(posDeleteTask, partitionType).get(MetadataColumns.FILE_PATH.fieldId()));
+ assertThat(filePartition).as("Expected correct partition on task's file").isEqualTo(0);
+ assertThat(taskPartition).as("Expected correct partition on task's column").isEqualTo(0);
+
+ assertThat(posDeleteTask.file().specId())
+ .as("Expected correct partition spec id on task")
+ .isEqualTo(0);
+ assertThat(constantsMap(posDeleteTask, partitionType).get(MetadataColumns.SPEC_ID.fieldId()))
Review Comment:
this should be using `.containsEntry()`. Same for the other places in this PR
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#issuecomment-1976907122
Thanks for the review. Will reflect on the comments.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1512157166
##########
core/src/test/java/org/apache/iceberg/TestIncrementalDataTableScan.java:
##########
@@ -196,56 +193,66 @@ public void testRollbacks() {
transaction.commitTransaction();
// Go back to snapshot "E"
table.manageSnapshots().rollbackTo(5).commit();
- Assert.assertEquals(5, table.currentSnapshot().snapshotId());
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(5);
filesMatch(Lists.newArrayList("B", "D", "E"), appendsBetweenScan(1, 5));
filesMatch(Lists.newArrayList("B", "D", "E"), appendsAfterScan(1));
}
- @Test
+ @TestTemplate
public void testIgnoreResiduals() throws IOException {
add(table.newAppend(), files("A"));
add(table.newAppend(), files("B"));
add(table.newAppend(), files("C"));
- TableScan scan1 = table.newScan().filter(Expressions.equal("id", 5)).appendsBetween(1, 3);
+ IncrementalAppendScan scan1 =
+ table
+ .newIncrementalAppendScan()
Review Comment:
`table.newScan(),appendsBetween` is marked as "deprecated" in https://iceberg.apache.org/javadoc/latest/org/apache/iceberg/TableScan.html so that I changed here.
> Deprecated.
since 1.0.0, will be removed in 2.0.0; use [Table.newIncrementalAppendScan()](https://iceberg.apache.org/javadoc/latest/org/apache/iceberg/Table.html#newIncrementalAppendScan()) instead.
But maybe this should be another PR, or keep it this time. Let me revert it to the previous version.
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#issuecomment-1981376570
@nastra Update with your comments. When you have time, could you review the new commit?
--
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
Re: [PR] Migrate Read in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #9849:
URL: https://github.com/apache/iceberg/pull/9849#discussion_r1512160426
##########
core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java:
##########
@@ -356,14 +353,14 @@ public void testPartitionsTableScanNoFilter() {
Types.StructType.of(optional(1000, "data_bucket", Types.IntegerType.get()))))
.asStruct();
TableScan scanNoFilter = partitionsTable.newScan().select("partition.data_bucket");
- Assert.assertEquals(expected, scanNoFilter.schema().asStruct());
+ assertThat(scanNoFilter.schema().asStruct()).isEqualTo(expected);
CloseableIterable<ManifestEntry<?>> entries =
PartitionsTable.planEntries((StaticTableScan) scanNoFilter);
if (formatVersion == 2) {
- Assert.assertEquals(8, Iterators.size(entries.iterator()));
+ assertThat(entries.iterator()).toIterable().hasSize(8);
Review Comment:
This and other parts can be `assertThat(entries).hasSize(x)`. Will replace them.
--
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