You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "cgpoh (via GitHub)" <gi...@apache.org> on 2023/11/30 08:45:25 UTC
[PR] chore: junit 5 migration for TestFlinkScan [iceberg]
cgpoh opened a new pull request, #9185:
URL: https://github.com/apache/iceberg/pull/9185
Fixes #9077
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1410762888
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -288,14 +294,13 @@ private static void assertEquals(
ByteBuffer bb = ByteBuffer.wrap((byte[]) actual);
long firstLong = bb.getLong();
long secondLong = bb.getLong();
- Assert.assertEquals(
- "UUID should be equal",
- expected.toString(),
- new UUID(firstLong, secondLong).toString());
+ Assertions.assertThat(new UUID(firstLong, secondLong).toString())
+ .as("UUID should be equal")
+ .isEqualTo(expected.toString());
break;
case FIXED:
Assertions.assertThat(expected).as("Should expect byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals("binary should be equal", (byte[]) expected, (byte[]) actual);
+ Assertions.assertThat((byte[]) actual).as("binary should be equal").isEqualTo(expected);
Review Comment:
Could we do chaining?
```
Assertions.assertThat(expected)
.as("Should expect byte[]")
.isInstanceOf(byte[].class)
.isEqualTo(actual);
```
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1416540648
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,37 +51,28 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
-
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
-
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
- @Rule
- public final HadoopCatalogResource catalogResource =
- new HadoopCatalogResource(TEMPORARY_FOLDER, TestFixtures.DATABASE, TestFixtures.TABLE);
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
+ MiniFlinkClusterExtension.createWithClassloaderCheckDisabled();
- // parametrized variables
- protected final FileFormat fileFormat;
+ @TempDir Path temporaryDirectory;
- @Parameterized.Parameters(name = "format={0}")
- public static Object[] parameters() {
- return new Object[] {"avro", "parquet", "orc"};
- }
+ @RegisterExtension
+ public final HadoopCatResource catalogResource =
+ new HadoopCatResource(temporaryDirectory, TestFixtures.DATABASE, TestFixtures.TABLE);
- TestFlinkScan(String fileFormat) {
- this.fileFormat = FileFormat.fromString(fileFormat);
+ public static Stream<Arguments> parameters() {
+ return Stream.of(
+ Arguments.of(FileFormat.fromString("avro")),
Review Comment:
Thanks @pvary !
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1410759259
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -193,75 +192,81 @@ private static void assertEquals(
return;
}
- Assert.assertTrue(
- "expected and actual should be both null or not null", expected != null && actual != null);
+ Assertions.assertThat(expected != null && actual != null)
Review Comment:
Maybe static import `Assertions.assertThat`?
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1877466639
> Do we have a way to check if the new tests were running on the CI? With the correct parameters?
I don't think so, but I was verifying them all locally
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1413797474
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,37 +51,28 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
-
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
-
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
- @Rule
- public final HadoopCatalogResource catalogResource =
- new HadoopCatalogResource(TEMPORARY_FOLDER, TestFixtures.DATABASE, TestFixtures.TABLE);
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
+ MiniFlinkClusterExtension.createWithClassloaderCheckDisabled();
- // parametrized variables
- protected final FileFormat fileFormat;
+ @TempDir Path temporaryDirectory;
- @Parameterized.Parameters(name = "format={0}")
- public static Object[] parameters() {
- return new Object[] {"avro", "parquet", "orc"};
- }
+ @RegisterExtension
+ public final HadoopCatResource catalogResource =
+ new HadoopCatResource(temporaryDirectory, TestFixtures.DATABASE, TestFixtures.TABLE);
- TestFlinkScan(String fileFormat) {
- this.fileFormat = FileFormat.fromString(fileFormat);
+ public static Stream<Arguments> parameters() {
+ return Stream.of(
+ Arguments.of(FileFormat.fromString("avro")),
Review Comment:
Could this be `FileFormat.PARQUET`, `FileFormat.ORC`, `FileFormat.AVRO`?
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434950138
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -337,82 +334,72 @@ private static void assertAvroEquals(
if (expected == null && actual == null) {
return;
}
-
- Assert.assertTrue(
- "expected and actual should be both null or not null", expected != null && actual != null);
+ assertThat(expected).isNotNull();
+ assertThat(actual).isNotNull();
switch (type.typeId()) {
case BOOLEAN:
case INTEGER:
case LONG:
case FLOAT:
case DOUBLE:
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect a " + type.typeId().javaClass())
.isInstanceOf(type.typeId().javaClass());
- Assertions.assertThat(actual)
+ assertThat(actual)
.as("Should expect a " + type.typeId().javaClass())
.isInstanceOf(type.typeId().javaClass());
- Assert.assertEquals(type.typeId() + " value should be equal", expected, actual);
+ assertThat(actual).as(type.typeId() + " value should be equal").isEqualTo(expected);
break;
case STRING:
- Assertions.assertThat(expected)
- .as("Should expect a CharSequence")
- .isInstanceOf(CharSequence.class);
- Assertions.assertThat(actual)
- .as("Should expect a CharSequence")
- .isInstanceOf(CharSequence.class);
- Assert.assertEquals("string should be equal", expected.toString(), actual.toString());
+ assertThat(expected).as("Should expect a CharSequence").isInstanceOf(CharSequence.class);
+ assertThat(actual).as("Should expect a CharSequence").isInstanceOf(CharSequence.class);
+ assertThat(actual.toString()).as("string should be equal").isEqualTo(expected.toString());
break;
case DATE:
- Assertions.assertThat(expected).as("Should expect a Date").isInstanceOf(LocalDate.class);
+ assertThat(expected).as("Should expect a Date").isInstanceOf(LocalDate.class);
LocalDate date = DateTimeUtil.dateFromDays((int) actual);
- Assert.assertEquals("date should be equal", expected, date);
+ assertThat(date).as("date should be equal").isEqualTo(expected);
break;
case TIME:
- Assertions.assertThat(expected)
- .as("Should expect a LocalTime")
- .isInstanceOf(LocalTime.class);
+ assertThat(expected).as("Should expect a LocalTime").isInstanceOf(LocalTime.class);
int milliseconds = (int) (((LocalTime) expected).toNanoOfDay() / 1000_000);
- Assert.assertEquals("time millis should be equal", milliseconds, actual);
+ assertThat(actual).as("time millis should be equal").isEqualTo(milliseconds);
break;
case TIMESTAMP:
if (((Types.TimestampType) type).shouldAdjustToUTC()) {
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect a OffsetDataTime")
.isInstanceOf(OffsetDateTime.class);
OffsetDateTime ts = (OffsetDateTime) expected;
- Assert.assertEquals(
- "OffsetDataTime should be equal",
- ts.toLocalDateTime(),
- ((TimestampData) actual).toLocalDateTime());
+ assertThat(((TimestampData) actual).toLocalDateTime())
+ .as("OffsetDataTime should be equal")
+ .isEqualTo(ts.toLocalDateTime());
} else {
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect a LocalDataTime")
.isInstanceOf(LocalDateTime.class);
LocalDateTime ts = (LocalDateTime) expected;
- Assert.assertEquals(
- "LocalDataTime should be equal", ts, ((TimestampData) actual).toLocalDateTime());
+ assertThat(((TimestampData) actual).toLocalDateTime())
+ .as("LocalDataTime should be equal")
+ .isEqualTo(ts);
}
break;
case BINARY:
- Assertions.assertThat(expected)
+ assertThat(expected)
Review Comment:
looks like the actual/expected is the wrong way here as well. Can you please check all the other places in this file to make sure that we're using `assertThat(actual)...isEqualTo(expected)
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434958256
##########
data/src/test/java/org/apache/iceberg/data/GenAppenderHelper.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AppendFiles;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.assertj.core.api.Assertions;
+
+/** Helper for appending {@link DataFile} to a table or appending {@link Record}s to a table. */
+public class GenAppenderHelper {
Review Comment:
I don't think we need this class. We can just update `GenericAppenderHelper` to work with JUnit4's `TemporaryFolder` and JUnit5's `Path` by applying this patch:
```
diff --git a/data/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java b/data/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java
index beccc8bb7..8521004ca 100644
--- a/data/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java
+++ b/data/src/test/java/org/apache/iceberg/data/GenericAppenderHelper.java
@@ -20,6 +20,7 @@ package org.apache.iceberg.data;
import java.io.File;
import java.io.IOException;
+import java.nio.file.Path;
import java.util.List;
import org.apache.hadoop.conf.Configuration;
import org.apache.iceberg.AppendFiles;
@@ -44,13 +45,24 @@ public class GenericAppenderHelper {
private final Table table;
private final FileFormat fileFormat;
private final TemporaryFolder tmp;
+ private final Path temp;
private final Configuration conf;
+ @Deprecated
public GenericAppenderHelper(
Table table, FileFormat fileFormat, TemporaryFolder tmp, Configuration conf) {
this.table = table;
this.fileFormat = fileFormat;
this.tmp = tmp;
+ this.temp = null;
+ this.conf = conf;
+ }
+
+ public GenericAppenderHelper(Table table, FileFormat fileFormat, Path temp, Configuration conf) {
+ this.table = table;
+ this.fileFormat = fileFormat;
+ this.tmp = null;
+ this.temp = temp;
this.conf = conf;
}
@@ -94,14 +106,14 @@ public class GenericAppenderHelper {
public DataFile writeFile(List<Record> records) throws IOException {
Preconditions.checkNotNull(table, "table not set");
- File file = tmp.newFile();
+ File file = null != tmp ? tmp.newFile() : File.createTempFile("junit", null, temp.toFile());
Assert.assertTrue(file.delete());
return appendToLocalFile(table, file, fileFormat, null, records, conf);
}
public DataFile writeFile(StructLike partition, List<Record> records) throws IOException {
Preconditions.checkNotNull(table, "table not set");
- File file = tmp.newFile();
+ File file = null != tmp ? tmp.newFile() : File.createTempFile("junit", null, temp.toFile());
Assert.assertTrue(file.delete());
return appendToLocalFile(table, file, fileFormat, partition, records, conf);
}
```
that way we don't have to maintain two files.
@cgpoh can you please remove `GenAppenderHelper` and apply the Assert -> AssertJ conversions to `GenericAppenderHelper`?
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1421376428
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -193,109 +192,106 @@ private static void assertEquals(
return;
}
- Assert.assertTrue(
- "expected and actual should be both null or not null", expected != null && actual != null);
+ assertThat(expected).isNotNull();
+ assertThat(actual).isNotNull();
Review Comment:
I think the description is just misleading, because the check actually makes sure that both are not null
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1867581096
@cgpoh I took the liberty to address the remaining items so that we can get this in. @pvary could you do a final review on this one please?
I thin we would also want to apply this to Flink 1.18
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1410770854
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkSrc.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink.source;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.flink.table.api.TableColumn;
+import org.apache.flink.table.api.TableSchema;
+import org.apache.flink.types.Row;
+import org.apache.iceberg.expressions.Expression;
+import org.apache.iceberg.flink.FlinkSchemaUtil;
+import org.apache.iceberg.flink.TestFixtures;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+public abstract class TestFlinkSrc extends TestScan {
Review Comment:
Same for `TestFlinkScan`
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1432509022
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -458,19 +457,20 @@ private void testFilterExp(Expression filter, String sqlFilter, boolean caseSens
TestHelpers.assertRecords(actual, expectedRecords.subList(1, 3), TestFixtures.SCHEMA);
}
- @Test
+ @TestTemplate
public void testFilterExp() throws Exception {
- testFilterExp(Expressions.greaterThanOrEqual("data", "b"), "where data>='b'", true);
+ testFilterExp(Expressions.greaterThanOrEqual("data", "b"), "where data>='b'", true, fileFormat);
Review Comment:
is this change necessary?
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1419643002
##########
data/src/test/java/org/apache/iceberg/data/GenAppenderHelper.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AppendFiles;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.assertj.core.api.Assertions;
+
+/** Helper for appending {@link DataFile} to a table or appending {@link Record}s to a table. */
+public class GenAppenderHelper {
Review Comment:
If that is the case, then we should keep this here. Thanks
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1414267995
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,37 +51,28 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
-
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
-
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
- @Rule
- public final HadoopCatalogResource catalogResource =
- new HadoopCatalogResource(TEMPORARY_FOLDER, TestFixtures.DATABASE, TestFixtures.TABLE);
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
+ MiniFlinkClusterExtension.createWithClassloaderCheckDisabled();
- // parametrized variables
- protected final FileFormat fileFormat;
+ @TempDir Path temporaryDirectory;
- @Parameterized.Parameters(name = "format={0}")
- public static Object[] parameters() {
- return new Object[] {"avro", "parquet", "orc"};
- }
+ @RegisterExtension
+ public final HadoopCatResource catalogResource =
+ new HadoopCatResource(temporaryDirectory, TestFixtures.DATABASE, TestFixtures.TABLE);
- TestFlinkScan(String fileFormat) {
- this.fileFormat = FileFormat.fromString(fileFormat);
+ public static Stream<Arguments> parameters() {
+ return Stream.of(
+ Arguments.of(FileFormat.fromString("avro")),
Review Comment:
Also please join the discussion on #9161 to decide a general way to implement parametrized 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
Re: [PR] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415431103
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -523,89 +513,102 @@ public static void assertEquals(ManifestFile expected, ManifestFile actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Should not be null.", expected != null && actual != null);
- Assert.assertEquals("Path must match", expected.path(), actual.path());
- Assert.assertEquals("Length must match", expected.length(), actual.length());
- Assert.assertEquals("Spec id must match", expected.partitionSpecId(), actual.partitionSpecId());
- Assert.assertEquals("ManifestContent must match", expected.content(), actual.content());
- Assert.assertEquals(
- "SequenceNumber must match", expected.sequenceNumber(), actual.sequenceNumber());
- Assert.assertEquals(
- "MinSequenceNumber must match", expected.minSequenceNumber(), actual.minSequenceNumber());
- Assert.assertEquals("Snapshot id must match", expected.snapshotId(), actual.snapshotId());
- Assert.assertEquals(
- "Added files flag must match", expected.hasAddedFiles(), actual.hasAddedFiles());
- Assert.assertEquals(
- "Added files count must match", expected.addedFilesCount(), actual.addedFilesCount());
- Assert.assertEquals(
- "Added rows count must match", expected.addedRowsCount(), actual.addedRowsCount());
- Assert.assertEquals(
- "Existing files flag must match", expected.hasExistingFiles(), actual.hasExistingFiles());
- Assert.assertEquals(
- "Existing files count must match",
- expected.existingFilesCount(),
- actual.existingFilesCount());
- Assert.assertEquals(
- "Existing rows count must match", expected.existingRowsCount(), actual.existingRowsCount());
- Assert.assertEquals(
- "Deleted files flag must match", expected.hasDeletedFiles(), actual.hasDeletedFiles());
- Assert.assertEquals(
- "Deleted files count must match", expected.deletedFilesCount(), actual.deletedFilesCount());
- Assert.assertEquals(
- "Deleted rows count must match", expected.deletedRowsCount(), actual.deletedRowsCount());
+ assertThat(expected != null && actual != null).as("Should not be null.").isTrue();
+ assertThat(actual.path()).as("Path must match").isEqualTo(expected.path());
+ assertThat(actual.length()).as("Length must match").isEqualTo(expected.length());
+ assertThat(actual.partitionSpecId())
+ .as("Spec id must match")
+ .isEqualTo(expected.partitionSpecId());
+ assertThat(actual.content()).as("ManifestContent must match").isEqualTo(expected.content());
+ assertThat(actual.sequenceNumber())
+ .as("SequenceNumber must match")
+ .isEqualTo(expected.sequenceNumber());
+ assertThat(actual.minSequenceNumber())
+ .as("MinSequenceNumber must match")
+ .isEqualTo(expected.minSequenceNumber());
+ assertThat(actual.snapshotId()).as("Snapshot id must match").isEqualTo(expected.snapshotId());
+ assertThat(actual.hasAddedFiles())
+ .as("Added files flag must match")
+ .isEqualTo(expected.hasAddedFiles());
+ assertThat(actual.addedFilesCount())
+ .as("Added files count must match")
+ .isEqualTo(expected.addedFilesCount());
+ assertThat(actual.addedRowsCount())
+ .as("Added rows count must match")
+ .isEqualTo(expected.addedRowsCount());
+ assertThat(actual.hasExistingFiles())
+ .as("Existing files flag must match")
+ .isEqualTo(expected.hasExistingFiles());
+ assertThat(actual.existingFilesCount())
+ .as("Existing files count must match")
+ .isEqualTo(expected.existingFilesCount());
+ assertThat(actual.existingRowsCount())
+ .as("Existing rows count must match")
+ .isEqualTo(expected.existingRowsCount());
+ assertThat(actual.hasDeletedFiles())
+ .as("Deleted files flag must match")
+ .isEqualTo(expected.hasDeletedFiles());
+ assertThat(actual.deletedFilesCount())
+ .as("Deleted files count must match")
+ .isEqualTo(expected.deletedFilesCount());
+ assertThat(actual.deletedRowsCount())
+ .as("Deleted rows count must match")
+ .isEqualTo(expected.deletedRowsCount());
List<ManifestFile.PartitionFieldSummary> expectedSummaries = expected.partitions();
List<ManifestFile.PartitionFieldSummary> actualSummaries = actual.partitions();
- Assert.assertEquals(
- "PartitionFieldSummary size does not match",
- expectedSummaries.size(),
- actualSummaries.size());
+ assertThat(actualSummaries.size())
+ .as("PartitionFieldSummary size does not match")
+ .isEqualTo(expectedSummaries.size());
for (int i = 0; i < expectedSummaries.size(); i++) {
- Assert.assertEquals(
- "Null flag in partition must match",
- expectedSummaries.get(i).containsNull(),
- actualSummaries.get(i).containsNull());
- Assert.assertEquals(
- "NaN flag in partition must match",
- expectedSummaries.get(i).containsNaN(),
- actualSummaries.get(i).containsNaN());
- Assert.assertEquals(
- "Lower bounds in partition must match",
- expectedSummaries.get(i).lowerBound(),
- actualSummaries.get(i).lowerBound());
- Assert.assertEquals(
- "Upper bounds in partition must match",
- expectedSummaries.get(i).upperBound(),
- actualSummaries.get(i).upperBound());
+ assertThat(actualSummaries.get(i).containsNull())
+ .as("Null flag in partition must match")
+ .isEqualTo(expectedSummaries.get(i).containsNull());
+ assertThat(actualSummaries.get(i).containsNaN())
+ .as("NaN flag in partition must match")
+ .isEqualTo(expectedSummaries.get(i).containsNaN());
+ assertThat(actualSummaries.get(i).lowerBound())
+ .as("Lower bounds in partition must match")
+ .isEqualTo(expectedSummaries.get(i).lowerBound());
+ assertThat(actualSummaries.get(i).upperBound())
+ .as("Upper bounds in partition must match")
+ .isEqualTo(expectedSummaries.get(i).upperBound());
}
}
public static void assertEquals(ContentFile<?> expected, ContentFile<?> actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Shouldn't be null.", expected != null && actual != null);
- Assert.assertEquals("SpecId", expected.specId(), actual.specId());
- Assert.assertEquals("Content", expected.content(), actual.content());
- Assert.assertEquals("Path", expected.path(), actual.path());
- Assert.assertEquals("Format", expected.format(), actual.format());
- Assert.assertEquals("Partition size", expected.partition().size(), actual.partition().size());
+ assertThat(expected != null && actual != null).as("Shouldn't be null.").isTrue();
Review Comment:
same as above
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -523,89 +513,102 @@ public static void assertEquals(ManifestFile expected, ManifestFile actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Should not be null.", expected != null && actual != null);
- Assert.assertEquals("Path must match", expected.path(), actual.path());
- Assert.assertEquals("Length must match", expected.length(), actual.length());
- Assert.assertEquals("Spec id must match", expected.partitionSpecId(), actual.partitionSpecId());
- Assert.assertEquals("ManifestContent must match", expected.content(), actual.content());
- Assert.assertEquals(
- "SequenceNumber must match", expected.sequenceNumber(), actual.sequenceNumber());
- Assert.assertEquals(
- "MinSequenceNumber must match", expected.minSequenceNumber(), actual.minSequenceNumber());
- Assert.assertEquals("Snapshot id must match", expected.snapshotId(), actual.snapshotId());
- Assert.assertEquals(
- "Added files flag must match", expected.hasAddedFiles(), actual.hasAddedFiles());
- Assert.assertEquals(
- "Added files count must match", expected.addedFilesCount(), actual.addedFilesCount());
- Assert.assertEquals(
- "Added rows count must match", expected.addedRowsCount(), actual.addedRowsCount());
- Assert.assertEquals(
- "Existing files flag must match", expected.hasExistingFiles(), actual.hasExistingFiles());
- Assert.assertEquals(
- "Existing files count must match",
- expected.existingFilesCount(),
- actual.existingFilesCount());
- Assert.assertEquals(
- "Existing rows count must match", expected.existingRowsCount(), actual.existingRowsCount());
- Assert.assertEquals(
- "Deleted files flag must match", expected.hasDeletedFiles(), actual.hasDeletedFiles());
- Assert.assertEquals(
- "Deleted files count must match", expected.deletedFilesCount(), actual.deletedFilesCount());
- Assert.assertEquals(
- "Deleted rows count must match", expected.deletedRowsCount(), actual.deletedRowsCount());
+ assertThat(expected != null && actual != null).as("Should not be null.").isTrue();
+ assertThat(actual.path()).as("Path must match").isEqualTo(expected.path());
+ assertThat(actual.length()).as("Length must match").isEqualTo(expected.length());
+ assertThat(actual.partitionSpecId())
+ .as("Spec id must match")
+ .isEqualTo(expected.partitionSpecId());
+ assertThat(actual.content()).as("ManifestContent must match").isEqualTo(expected.content());
+ assertThat(actual.sequenceNumber())
+ .as("SequenceNumber must match")
+ .isEqualTo(expected.sequenceNumber());
+ assertThat(actual.minSequenceNumber())
+ .as("MinSequenceNumber must match")
+ .isEqualTo(expected.minSequenceNumber());
+ assertThat(actual.snapshotId()).as("Snapshot id must match").isEqualTo(expected.snapshotId());
+ assertThat(actual.hasAddedFiles())
+ .as("Added files flag must match")
+ .isEqualTo(expected.hasAddedFiles());
+ assertThat(actual.addedFilesCount())
+ .as("Added files count must match")
+ .isEqualTo(expected.addedFilesCount());
+ assertThat(actual.addedRowsCount())
+ .as("Added rows count must match")
+ .isEqualTo(expected.addedRowsCount());
+ assertThat(actual.hasExistingFiles())
+ .as("Existing files flag must match")
+ .isEqualTo(expected.hasExistingFiles());
+ assertThat(actual.existingFilesCount())
+ .as("Existing files count must match")
+ .isEqualTo(expected.existingFilesCount());
+ assertThat(actual.existingRowsCount())
+ .as("Existing rows count must match")
+ .isEqualTo(expected.existingRowsCount());
+ assertThat(actual.hasDeletedFiles())
+ .as("Deleted files flag must match")
+ .isEqualTo(expected.hasDeletedFiles());
+ assertThat(actual.deletedFilesCount())
+ .as("Deleted files count must match")
+ .isEqualTo(expected.deletedFilesCount());
+ assertThat(actual.deletedRowsCount())
+ .as("Deleted rows count must match")
+ .isEqualTo(expected.deletedRowsCount());
List<ManifestFile.PartitionFieldSummary> expectedSummaries = expected.partitions();
List<ManifestFile.PartitionFieldSummary> actualSummaries = actual.partitions();
- Assert.assertEquals(
- "PartitionFieldSummary size does not match",
- expectedSummaries.size(),
- actualSummaries.size());
+ assertThat(actualSummaries.size())
+ .as("PartitionFieldSummary size does not match")
+ .isEqualTo(expectedSummaries.size());
for (int i = 0; i < expectedSummaries.size(); i++) {
- Assert.assertEquals(
- "Null flag in partition must match",
- expectedSummaries.get(i).containsNull(),
- actualSummaries.get(i).containsNull());
- Assert.assertEquals(
- "NaN flag in partition must match",
- expectedSummaries.get(i).containsNaN(),
- actualSummaries.get(i).containsNaN());
- Assert.assertEquals(
- "Lower bounds in partition must match",
- expectedSummaries.get(i).lowerBound(),
- actualSummaries.get(i).lowerBound());
- Assert.assertEquals(
- "Upper bounds in partition must match",
- expectedSummaries.get(i).upperBound(),
- actualSummaries.get(i).upperBound());
+ assertThat(actualSummaries.get(i).containsNull())
+ .as("Null flag in partition must match")
+ .isEqualTo(expectedSummaries.get(i).containsNull());
+ assertThat(actualSummaries.get(i).containsNaN())
+ .as("NaN flag in partition must match")
+ .isEqualTo(expectedSummaries.get(i).containsNaN());
+ assertThat(actualSummaries.get(i).lowerBound())
+ .as("Lower bounds in partition must match")
+ .isEqualTo(expectedSummaries.get(i).lowerBound());
+ assertThat(actualSummaries.get(i).upperBound())
+ .as("Upper bounds in partition must match")
+ .isEqualTo(expectedSummaries.get(i).upperBound());
}
}
public static void assertEquals(ContentFile<?> expected, ContentFile<?> actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Shouldn't be null.", expected != null && actual != null);
- Assert.assertEquals("SpecId", expected.specId(), actual.specId());
- Assert.assertEquals("Content", expected.content(), actual.content());
- Assert.assertEquals("Path", expected.path(), actual.path());
- Assert.assertEquals("Format", expected.format(), actual.format());
- Assert.assertEquals("Partition size", expected.partition().size(), actual.partition().size());
+ assertThat(expected != null && actual != null).as("Shouldn't be null.").isTrue();
+ assertThat(actual.specId()).as("SpecId").isEqualTo(expected.specId());
+ assertThat(actual.content()).as("Content").isEqualTo(expected.content());
+ assertThat(actual.path()).as("Path").isEqualTo(expected.path());
+ assertThat(actual.format()).as("Format").isEqualTo(expected.format());
+ assertThat(actual.partition().size())
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415406872
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -309,8 +307,8 @@ public static void assertEquals(Schema schema, List<GenericData.Record> records,
public static void assertEquals(Schema schema, GenericData.Record record, Row row) {
List<Types.NestedField> fields = schema.asStruct().fields();
- Assert.assertEquals(fields.size(), record.getSchema().getFields().size());
- Assert.assertEquals(fields.size(), row.getArity());
+ assertThat(fields.size()).isEqualTo(record.getSchema().getFields().size());
Review Comment:
`assertThat(xyz).hasSameSizeAs(...)`
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1870902422
> @cgpoh I took the liberty to address the remaining items so that we can get this in. @pvary could you do a final review on this one please? I think we would also want to apply this to Flink 1.18
Thanks @nastra for addressing the remaining items. I don’t have access to my laptop for now until new year :)
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "rodmeneses (via GitHub)" <gi...@apache.org>.
rodmeneses commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1420794807
##########
data/src/test/java/org/apache/iceberg/data/GenAppenderHelper.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AppendFiles;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.assertj.core.api.Assertions;
+
+/** Helper for appending {@link DataFile} to a table or appending {@link Record}s to a table. */
+public class GenAppenderHelper {
+
+ private static final String ORC_CONFIG_PREFIX = "^orc.*";
+ private static final String PARQUET_CONFIG_PATTERN = ".*parquet.*";
+
+ private static final String TEMP_FILE = "temp";
+
+ private final Table table;
+ private final FileFormat fileFormat;
+ private final Path tmp;
+ private final Configuration conf;
+
+ public GenAppenderHelper(Table table, FileFormat fileFormat, Path tmp, Configuration conf) {
+ this.table = table;
+ this.fileFormat = fileFormat;
+ this.tmp = tmp;
+ this.conf = conf;
+ }
+
+ public GenAppenderHelper(Table table, FileFormat fileFormat, Path tmp) {
+ this(table, fileFormat, tmp, null);
+ }
+
+ public void appendToTable(String branch, DataFile... dataFiles) {
+ Preconditions.checkNotNull(table, "table not set");
+
+ AppendFiles append =
+ table.newAppend().toBranch(branch != null ? branch : SnapshotRef.MAIN_BRANCH);
+
+ for (DataFile dataFile : dataFiles) {
+ append = append.appendFile(dataFile);
+ }
+
+ append.commit();
+ }
+
+ public void appendToTable(DataFile... dataFiles) {
+ appendToTable(null, dataFiles);
+ }
+
+ public void appendToTable(List<Record> records) throws IOException {
+ appendToTable(null, null, records);
+ }
+
+ public void appendToTable(String branch, List<Record> records) throws IOException {
+ appendToTable(null, branch, records);
+ }
+
+ public void appendToTable(StructLike partition, String branch, List<Record> records)
+ throws IOException {
+ appendToTable(branch, writeFile(partition, records));
+ }
+
+ public void appendToTable(StructLike partition, List<Record> records) throws IOException {
+ appendToTable(writeFile(partition, records));
+ }
+
+ public DataFile writeFile(List<Record> records) throws IOException {
+ Preconditions.checkNotNull(table, "table not set");
+ File file = File.createTempFile(TEMP_FILE, null);
+ Assertions.assertThat(file.delete()).isTrue();
+ return appendToLocalFile(table, file, fileFormat, null, records, conf);
+ }
+
+ public DataFile writeFile(StructLike partition, List<Record> records) throws IOException {
+ Preconditions.checkNotNull(table, "table not set");
+ File file = File.createTempFile(TEMP_FILE, null);
+ Assertions.assertThat(file.delete()).isTrue();
+ return appendToLocalFile(table, file, fileFormat, partition, records, conf);
+ }
+
+ private static DataFile appendToLocalFile(
+ Table table,
+ File file,
+ FileFormat format,
+ StructLike partition,
+ List<Record> records,
+ Configuration conf)
+ throws IOException {
+ GenericAppenderFactory appenderFactory = new GenericAppenderFactory(table.schema());
+
+ // Push down ORC related settings to appender if there are any
+ if (FileFormat.ORC.equals(format) && conf != null) {
+ appenderFactory.setAll(conf.getValByRegex(ORC_CONFIG_PREFIX));
+ }
+
+ if (FileFormat.PARQUET.equals(format) && conf != null) {
+ appenderFactory.setAll(conf.getValByRegex(PARQUET_CONFIG_PATTERN));
+ }
+
+ FileAppender<Record> appender = appenderFactory.newAppender(Files.localOutput(file), format);
+ try (FileAppender<Record> fileAppender = appender) {
+ fileAppender.addAll(records);
Review Comment:
should we log if we have exceptions?
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1418251447
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -523,89 +513,102 @@ public static void assertEquals(ManifestFile expected, ManifestFile actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Should not be null.", expected != null && actual != null);
- Assert.assertEquals("Path must match", expected.path(), actual.path());
- Assert.assertEquals("Length must match", expected.length(), actual.length());
- Assert.assertEquals("Spec id must match", expected.partitionSpecId(), actual.partitionSpecId());
- Assert.assertEquals("ManifestContent must match", expected.content(), actual.content());
- Assert.assertEquals(
- "SequenceNumber must match", expected.sequenceNumber(), actual.sequenceNumber());
- Assert.assertEquals(
- "MinSequenceNumber must match", expected.minSequenceNumber(), actual.minSequenceNumber());
- Assert.assertEquals("Snapshot id must match", expected.snapshotId(), actual.snapshotId());
- Assert.assertEquals(
- "Added files flag must match", expected.hasAddedFiles(), actual.hasAddedFiles());
- Assert.assertEquals(
- "Added files count must match", expected.addedFilesCount(), actual.addedFilesCount());
- Assert.assertEquals(
- "Added rows count must match", expected.addedRowsCount(), actual.addedRowsCount());
- Assert.assertEquals(
- "Existing files flag must match", expected.hasExistingFiles(), actual.hasExistingFiles());
- Assert.assertEquals(
- "Existing files count must match",
- expected.existingFilesCount(),
- actual.existingFilesCount());
- Assert.assertEquals(
- "Existing rows count must match", expected.existingRowsCount(), actual.existingRowsCount());
- Assert.assertEquals(
- "Deleted files flag must match", expected.hasDeletedFiles(), actual.hasDeletedFiles());
- Assert.assertEquals(
- "Deleted files count must match", expected.deletedFilesCount(), actual.deletedFilesCount());
- Assert.assertEquals(
- "Deleted rows count must match", expected.deletedRowsCount(), actual.deletedRowsCount());
+ assertThat(expected != null && actual != null).as("Should not be null.").isTrue();
+ assertThat(actual.path()).as("Path must match").isEqualTo(expected.path());
+ assertThat(actual.length()).as("Length must match").isEqualTo(expected.length());
+ assertThat(actual.partitionSpecId())
+ .as("Spec id must match")
+ .isEqualTo(expected.partitionSpecId());
+ assertThat(actual.content()).as("ManifestContent must match").isEqualTo(expected.content());
+ assertThat(actual.sequenceNumber())
+ .as("SequenceNumber must match")
+ .isEqualTo(expected.sequenceNumber());
+ assertThat(actual.minSequenceNumber())
+ .as("MinSequenceNumber must match")
+ .isEqualTo(expected.minSequenceNumber());
+ assertThat(actual.snapshotId()).as("Snapshot id must match").isEqualTo(expected.snapshotId());
+ assertThat(actual.hasAddedFiles())
+ .as("Added files flag must match")
+ .isEqualTo(expected.hasAddedFiles());
+ assertThat(actual.addedFilesCount())
+ .as("Added files count must match")
+ .isEqualTo(expected.addedFilesCount());
+ assertThat(actual.addedRowsCount())
+ .as("Added rows count must match")
+ .isEqualTo(expected.addedRowsCount());
+ assertThat(actual.hasExistingFiles())
+ .as("Existing files flag must match")
+ .isEqualTo(expected.hasExistingFiles());
+ assertThat(actual.existingFilesCount())
+ .as("Existing files count must match")
+ .isEqualTo(expected.existingFilesCount());
+ assertThat(actual.existingRowsCount())
+ .as("Existing rows count must match")
+ .isEqualTo(expected.existingRowsCount());
+ assertThat(actual.hasDeletedFiles())
+ .as("Deleted files flag must match")
+ .isEqualTo(expected.hasDeletedFiles());
+ assertThat(actual.deletedFilesCount())
+ .as("Deleted files count must match")
+ .isEqualTo(expected.deletedFilesCount());
+ assertThat(actual.deletedRowsCount())
+ .as("Deleted rows count must match")
+ .isEqualTo(expected.deletedRowsCount());
List<ManifestFile.PartitionFieldSummary> expectedSummaries = expected.partitions();
List<ManifestFile.PartitionFieldSummary> actualSummaries = actual.partitions();
- Assert.assertEquals(
- "PartitionFieldSummary size does not match",
- expectedSummaries.size(),
- actualSummaries.size());
+ assertThat(actualSummaries.size())
+ .as("PartitionFieldSummary size does not match")
+ .isEqualTo(expectedSummaries.size());
for (int i = 0; i < expectedSummaries.size(); i++) {
- Assert.assertEquals(
- "Null flag in partition must match",
- expectedSummaries.get(i).containsNull(),
- actualSummaries.get(i).containsNull());
- Assert.assertEquals(
- "NaN flag in partition must match",
- expectedSummaries.get(i).containsNaN(),
- actualSummaries.get(i).containsNaN());
- Assert.assertEquals(
- "Lower bounds in partition must match",
- expectedSummaries.get(i).lowerBound(),
- actualSummaries.get(i).lowerBound());
- Assert.assertEquals(
- "Upper bounds in partition must match",
- expectedSummaries.get(i).upperBound(),
- actualSummaries.get(i).upperBound());
+ assertThat(actualSummaries.get(i).containsNull())
+ .as("Null flag in partition must match")
+ .isEqualTo(expectedSummaries.get(i).containsNull());
+ assertThat(actualSummaries.get(i).containsNaN())
+ .as("NaN flag in partition must match")
+ .isEqualTo(expectedSummaries.get(i).containsNaN());
+ assertThat(actualSummaries.get(i).lowerBound())
+ .as("Lower bounds in partition must match")
+ .isEqualTo(expectedSummaries.get(i).lowerBound());
+ assertThat(actualSummaries.get(i).upperBound())
+ .as("Upper bounds in partition must match")
+ .isEqualTo(expectedSummaries.get(i).upperBound());
}
}
public static void assertEquals(ContentFile<?> expected, ContentFile<?> actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Shouldn't be null.", expected != null && actual != null);
- Assert.assertEquals("SpecId", expected.specId(), actual.specId());
- Assert.assertEquals("Content", expected.content(), actual.content());
- Assert.assertEquals("Path", expected.path(), actual.path());
- Assert.assertEquals("Format", expected.format(), actual.format());
- Assert.assertEquals("Partition size", expected.partition().size(), actual.partition().size());
+ assertThat(expected != null && actual != null).as("Shouldn't be null.").isTrue();
+ assertThat(actual.specId()).as("SpecId").isEqualTo(expected.specId());
+ assertThat(actual.content()).as("Content").isEqualTo(expected.content());
+ assertThat(actual.path()).as("Path").isEqualTo(expected.path());
+ assertThat(actual.format()).as("Format").isEqualTo(expected.format());
+ assertThat(actual.partition().size())
Review Comment:
@nastra , I have problem using `hasSameSizeAs(...)` here as actual.partition() is an implementation of the `StructLike` interface and is not extending from `Collection`, `assertThat` has difficulty inferring that `actual.partition()` can be enumerated.
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1878559904
@cgpoh can you please port these changes to other Flink versions?
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1432597940
##########
data/src/test/java/org/apache/iceberg/data/GenAppenderHelper.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AppendFiles;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.assertj.core.api.Assertions;
+
+/** Helper for appending {@link DataFile} to a table or appending {@link Record}s to a table. */
+public class GenAppenderHelper {
+
+ private static final String ORC_CONFIG_PREFIX = "^orc.*";
+ private static final String PARQUET_CONFIG_PATTERN = ".*parquet.*";
+
+ private static final String TEMP_FILE = "temp";
+
+ private final Table table;
+ private final FileFormat fileFormat;
+ private final Path tmp;
+ private final Configuration conf;
+
+ public GenAppenderHelper(Table table, FileFormat fileFormat, Path tmp, Configuration conf) {
+ this.table = table;
+ this.fileFormat = fileFormat;
+ this.tmp = tmp;
+ this.conf = conf;
+ }
+
+ public GenAppenderHelper(Table table, FileFormat fileFormat, Path tmp) {
+ this(table, fileFormat, tmp, null);
+ }
+
+ public void appendToTable(String branch, DataFile... dataFiles) {
+ Preconditions.checkNotNull(table, "table not set");
+
+ AppendFiles append =
+ table.newAppend().toBranch(branch != null ? branch : SnapshotRef.MAIN_BRANCH);
+
+ for (DataFile dataFile : dataFiles) {
+ append = append.appendFile(dataFile);
+ }
+
+ append.commit();
+ }
+
+ public void appendToTable(DataFile... dataFiles) {
+ appendToTable(null, dataFiles);
+ }
+
+ public void appendToTable(List<Record> records) throws IOException {
+ appendToTable(null, null, records);
+ }
+
+ public void appendToTable(String branch, List<Record> records) throws IOException {
+ appendToTable(null, branch, records);
+ }
+
+ public void appendToTable(StructLike partition, String branch, List<Record> records)
+ throws IOException {
+ appendToTable(branch, writeFile(partition, records));
+ }
+
+ public void appendToTable(StructLike partition, List<Record> records) throws IOException {
+ appendToTable(writeFile(partition, records));
+ }
+
+ public DataFile writeFile(List<Record> records) throws IOException {
+ Preconditions.checkNotNull(table, "table not set");
+ File file = File.createTempFile(TEMP_FILE, null);
+ Assertions.assertThat(file.delete()).isTrue();
+ return appendToLocalFile(table, file, fileFormat, null, records, conf);
+ }
+
+ public DataFile writeFile(StructLike partition, List<Record> records) throws IOException {
+ Preconditions.checkNotNull(table, "table not set");
+ File file = File.createTempFile(TEMP_FILE, null);
Review Comment:
shouldn't this be using `tmp`?
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415401117
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -159,9 +159,9 @@ public static void assertRowData(
return;
}
- Assert.assertTrue(
- "expected Record and actual RowData should be both null or not null",
- expectedRecord != null && actualRowData != null);
+ assertThat(expectedRecord != null && actualRowData != null)
Review Comment:
this should be `assertThat(expectedRecord).isNotNull()` and then assertThat(actualRowData).isNotNull()`.
We want to avoid using isTrue/isFalse as much as possible, because it's more difficult to debug when the assertion fails
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -193,109 +193,107 @@ private static void assertEquals(
return;
}
- Assert.assertTrue(
- "expected and actual should be both null or not null", expected != null && actual != null);
+ assertThat(expected != null && actual != null)
+ .as("expected and actual should be both null or not null")
+ .isTrue();
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1413800152
##########
data/src/test/java/org/apache/iceberg/data/GenAppenderHelper.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AppendFiles;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.assertj.core.api.Assertions;
+
+/** Helper for appending {@link DataFile} to a table or appending {@link Record}s to a table. */
+public class GenAppenderHelper {
Review Comment:
This is more like a question...
If I understand correctly `GenericAppenderHelper` is only used by `Flink` tests.
If we do not see it used in other places, we might want to consider it moving out from here, to the Flink specific code, and maybe with a package switch we can keep the class 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
Re: [PR] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1420194979
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,37 +51,28 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
Review Comment:
I brought this up on the [DEV list](https://lists.apache.org/thread/5c4xs9w4jrot57dknq6vvwyg4o069kmo). Let's give people a few days and then we can come back with the right approach to properly migrate this.
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434965278
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,41 +53,36 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
+ MiniFlinkClusterExtension.createWithClassloaderCheckDisabled();
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();
+ @TempDir Path temporaryDirectory;
- @Rule
- public final HadoopCatalogResource catalogResource =
- new HadoopCatalogResource(TEMPORARY_FOLDER, TestFixtures.DATABASE, TestFixtures.TABLE);
+ @RegisterExtension
+ public static final HadoopCatalogExtension catalogExtension =
Review Comment:
```suggestion
protected static final HadoopCatalogExtension catalogExtension =
```
--
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] Flink 1.17: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #9185:
URL: https://github.com/apache/iceberg/pull/9185
--
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] Flink 1.17: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1902586061
@nastra, thanks for the backport. Sorry that I'm not able to work on it earlier.
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1410758396
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.io.TempDir;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
+ protected final @TempDir Path temporaryFolder;
Review Comment:
Do we need the annotation here?
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.io.TempDir;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
+ protected final @TempDir Path temporaryFolder;
+ protected final String database;
+ protected final String tableName;
+
+ protected Catalog catalog;
+ protected CatalogLoader catalogLoader;
+ protected String warehouse;
+ protected TableLoader tableLoader;
+
+ public HadoopCatResource(@TempDir Path temporaryFolder, String database, String tableName) {
Review Comment:
Do we need the annotation 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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1413788323
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
Review Comment:
Isn't `HadoopCatalogExtension` a substitute for this?
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415518602
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
Review Comment:
yes I think we can just use `HadoopCatalogExtension`
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415430122
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -523,89 +513,102 @@ public static void assertEquals(ManifestFile expected, ManifestFile actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Should not be null.", expected != null && actual != null);
- Assert.assertEquals("Path must match", expected.path(), actual.path());
- Assert.assertEquals("Length must match", expected.length(), actual.length());
- Assert.assertEquals("Spec id must match", expected.partitionSpecId(), actual.partitionSpecId());
- Assert.assertEquals("ManifestContent must match", expected.content(), actual.content());
- Assert.assertEquals(
- "SequenceNumber must match", expected.sequenceNumber(), actual.sequenceNumber());
- Assert.assertEquals(
- "MinSequenceNumber must match", expected.minSequenceNumber(), actual.minSequenceNumber());
- Assert.assertEquals("Snapshot id must match", expected.snapshotId(), actual.snapshotId());
- Assert.assertEquals(
- "Added files flag must match", expected.hasAddedFiles(), actual.hasAddedFiles());
- Assert.assertEquals(
- "Added files count must match", expected.addedFilesCount(), actual.addedFilesCount());
- Assert.assertEquals(
- "Added rows count must match", expected.addedRowsCount(), actual.addedRowsCount());
- Assert.assertEquals(
- "Existing files flag must match", expected.hasExistingFiles(), actual.hasExistingFiles());
- Assert.assertEquals(
- "Existing files count must match",
- expected.existingFilesCount(),
- actual.existingFilesCount());
- Assert.assertEquals(
- "Existing rows count must match", expected.existingRowsCount(), actual.existingRowsCount());
- Assert.assertEquals(
- "Deleted files flag must match", expected.hasDeletedFiles(), actual.hasDeletedFiles());
- Assert.assertEquals(
- "Deleted files count must match", expected.deletedFilesCount(), actual.deletedFilesCount());
- Assert.assertEquals(
- "Deleted rows count must match", expected.deletedRowsCount(), actual.deletedRowsCount());
+ assertThat(expected != null && actual != null).as("Should not be null.").isTrue();
+ assertThat(actual.path()).as("Path must match").isEqualTo(expected.path());
+ assertThat(actual.length()).as("Length must match").isEqualTo(expected.length());
+ assertThat(actual.partitionSpecId())
+ .as("Spec id must match")
+ .isEqualTo(expected.partitionSpecId());
+ assertThat(actual.content()).as("ManifestContent must match").isEqualTo(expected.content());
+ assertThat(actual.sequenceNumber())
+ .as("SequenceNumber must match")
+ .isEqualTo(expected.sequenceNumber());
+ assertThat(actual.minSequenceNumber())
+ .as("MinSequenceNumber must match")
+ .isEqualTo(expected.minSequenceNumber());
+ assertThat(actual.snapshotId()).as("Snapshot id must match").isEqualTo(expected.snapshotId());
+ assertThat(actual.hasAddedFiles())
+ .as("Added files flag must match")
+ .isEqualTo(expected.hasAddedFiles());
+ assertThat(actual.addedFilesCount())
+ .as("Added files count must match")
+ .isEqualTo(expected.addedFilesCount());
+ assertThat(actual.addedRowsCount())
+ .as("Added rows count must match")
+ .isEqualTo(expected.addedRowsCount());
+ assertThat(actual.hasExistingFiles())
+ .as("Existing files flag must match")
+ .isEqualTo(expected.hasExistingFiles());
+ assertThat(actual.existingFilesCount())
+ .as("Existing files count must match")
+ .isEqualTo(expected.existingFilesCount());
+ assertThat(actual.existingRowsCount())
+ .as("Existing rows count must match")
+ .isEqualTo(expected.existingRowsCount());
+ assertThat(actual.hasDeletedFiles())
+ .as("Deleted files flag must match")
+ .isEqualTo(expected.hasDeletedFiles());
+ assertThat(actual.deletedFilesCount())
+ .as("Deleted files count must match")
+ .isEqualTo(expected.deletedFilesCount());
+ assertThat(actual.deletedRowsCount())
+ .as("Deleted rows count must match")
+ .isEqualTo(expected.deletedRowsCount());
List<ManifestFile.PartitionFieldSummary> expectedSummaries = expected.partitions();
List<ManifestFile.PartitionFieldSummary> actualSummaries = actual.partitions();
- Assert.assertEquals(
- "PartitionFieldSummary size does not match",
- expectedSummaries.size(),
- actualSummaries.size());
+ assertThat(actualSummaries.size())
Review Comment:
this should use hasSameSizeAs()
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434948333
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -193,109 +192,106 @@ private static void assertEquals(
return;
}
- Assert.assertTrue(
- "expected and actual should be both null or not null", expected != null && actual != null);
+ assertThat(expected).isNotNull();
+ assertThat(actual).isNotNull();
switch (type.typeId()) {
case BOOLEAN:
- Assert.assertEquals("boolean value should be equal", expected, actual);
+ assertThat(actual).as("boolean value should be equal").isEqualTo(expected);
break;
case INTEGER:
- Assert.assertEquals("int value should be equal", expected, actual);
+ assertThat(actual).as("int value should be equal").isEqualTo(expected);
break;
case LONG:
- Assert.assertEquals("long value should be equal", expected, actual);
+ assertThat(actual).as("long value should be equal").isEqualTo(expected);
break;
case FLOAT:
- Assert.assertEquals("float value should be equal", expected, actual);
+ assertThat(actual).as("float value should be equal").isEqualTo(expected);
break;
case DOUBLE:
- Assert.assertEquals("double value should be equal", expected, actual);
+ assertThat(actual).as("double value should be equal").isEqualTo(expected);
break;
case STRING:
- Assertions.assertThat(expected)
- .as("Should expect a CharSequence")
- .isInstanceOf(CharSequence.class);
- Assert.assertEquals("string should be equal", String.valueOf(expected), actual.toString());
+ assertThat(expected).as("Should expect a CharSequence").isInstanceOf(CharSequence.class);
+ assertThat(actual.toString())
+ .as("string should be equal")
+ .isEqualTo(String.valueOf(expected));
break;
case DATE:
- Assertions.assertThat(expected).as("Should expect a Date").isInstanceOf(LocalDate.class);
+ assertThat(expected).as("Should expect a Date").isInstanceOf(LocalDate.class);
LocalDate date = DateTimeUtil.dateFromDays((int) actual);
- Assert.assertEquals("date should be equal", expected, date);
+ assertThat(date).as("date should be equal").isEqualTo(expected);
break;
case TIME:
- Assertions.assertThat(expected)
- .as("Should expect a LocalTime")
- .isInstanceOf(LocalTime.class);
+ assertThat(expected).as("Should expect a LocalTime").isInstanceOf(LocalTime.class);
int milliseconds = (int) (((LocalTime) expected).toNanoOfDay() / 1000_000);
- Assert.assertEquals("time millis should be equal", milliseconds, actual);
+ assertThat(actual).as("time millis should be equal").isEqualTo(milliseconds);
break;
case TIMESTAMP:
if (((Types.TimestampType) type).shouldAdjustToUTC()) {
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect a OffsetDataTime")
.isInstanceOf(OffsetDateTime.class);
OffsetDateTime ts = (OffsetDateTime) expected;
- Assert.assertEquals(
- "OffsetDataTime should be equal",
- ts.toLocalDateTime(),
- ((TimestampData) actual).toLocalDateTime());
+ assertThat(((TimestampData) actual).toLocalDateTime())
+ .as("OffsetDataTime should be equal")
+ .isEqualTo(ts.toLocalDateTime());
} else {
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect a LocalDataTime")
.isInstanceOf(LocalDateTime.class);
LocalDateTime ts = (LocalDateTime) expected;
- Assert.assertEquals(
- "LocalDataTime should be equal", ts, ((TimestampData) actual).toLocalDateTime());
+ assertThat(((TimestampData) actual).toLocalDateTime())
+ .as("LocalDataTime should be equal")
+ .isEqualTo(ts);
}
break;
case BINARY:
- Assertions.assertThat(expected)
+ assertThat(expected)
.as("Should expect a ByteBuffer")
- .isInstanceOf(ByteBuffer.class);
- Assert.assertEquals("binary should be equal", expected, ByteBuffer.wrap((byte[]) actual));
+ .isInstanceOf(ByteBuffer.class)
+ .isEqualTo(ByteBuffer.wrap((byte[]) actual));
break;
case DECIMAL:
- Assertions.assertThat(expected)
- .as("Should expect a BigDecimal")
- .isInstanceOf(BigDecimal.class);
+ assertThat(expected).as("Should expect a BigDecimal").isInstanceOf(BigDecimal.class);
BigDecimal bd = (BigDecimal) expected;
- Assert.assertEquals(
- "decimal value should be equal", bd, ((DecimalData) actual).toBigDecimal());
+ assertThat(((DecimalData) actual).toBigDecimal())
+ .as("decimal value should be equal")
+ .isEqualTo(bd);
break;
case LIST:
- Assertions.assertThat(expected)
- .as("Should expect a Collection")
- .isInstanceOf(Collection.class);
+ assertThat(expected).as("Should expect a Collection").isInstanceOf(Collection.class);
Collection<?> expectedArrayData = (Collection<?>) expected;
ArrayData actualArrayData = (ArrayData) actual;
LogicalType elementType = ((ArrayType) logicalType).getElementType();
- Assert.assertEquals(
- "array length should be equal", expectedArrayData.size(), actualArrayData.size());
+ assertThat(actualArrayData.size())
+ .as("array length should be equal")
+ .isEqualTo(expectedArrayData.size());
assertArrayValues(
type.asListType().elementType(), elementType, expectedArrayData, actualArrayData);
break;
case MAP:
- Assertions.assertThat(expected).as("Should expect a Map").isInstanceOf(Map.class);
+ assertThat(expected).as("Should expect a Map").isInstanceOf(Map.class);
assertMapValues(type.asMapType(), logicalType, (Map<?, ?>) expected, (MapData) actual);
break;
case STRUCT:
- Assertions.assertThat(expected).as("Should expect a Record").isInstanceOf(StructLike.class);
+ assertThat(expected).as("Should expect a Record").isInstanceOf(StructLike.class);
assertRowData(type.asStructType(), logicalType, (StructLike) expected, (RowData) actual);
break;
case UUID:
- Assertions.assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
+ assertThat(expected).as("Should expect a UUID").isInstanceOf(UUID.class);
ByteBuffer bb = ByteBuffer.wrap((byte[]) actual);
long firstLong = bb.getLong();
long secondLong = bb.getLong();
- Assert.assertEquals(
- "UUID should be equal",
- expected.toString(),
- new UUID(firstLong, secondLong).toString());
+ assertThat(new UUID(firstLong, secondLong).toString())
+ .as("UUID should be equal")
+ .isEqualTo(expected.toString());
break;
case FIXED:
- Assertions.assertThat(expected).as("Should expect byte[]").isInstanceOf(byte[].class);
- Assert.assertArrayEquals("binary should be equal", (byte[]) expected, (byte[]) actual);
+ assertThat(expected)
Review Comment:
actual/expected is the wrong way 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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1432506498
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,41 +53,36 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
Review Comment:
is this one actually used by subclasses? if not, then we could reduce the visibility. Same for the other extension
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1877465254
Do we have a way to check if the new tests were running on the CI? With the correct 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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415428148
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -523,89 +513,102 @@ public static void assertEquals(ManifestFile expected, ManifestFile actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Should not be null.", expected != null && actual != null);
- Assert.assertEquals("Path must match", expected.path(), actual.path());
- Assert.assertEquals("Length must match", expected.length(), actual.length());
- Assert.assertEquals("Spec id must match", expected.partitionSpecId(), actual.partitionSpecId());
- Assert.assertEquals("ManifestContent must match", expected.content(), actual.content());
- Assert.assertEquals(
- "SequenceNumber must match", expected.sequenceNumber(), actual.sequenceNumber());
- Assert.assertEquals(
- "MinSequenceNumber must match", expected.minSequenceNumber(), actual.minSequenceNumber());
- Assert.assertEquals("Snapshot id must match", expected.snapshotId(), actual.snapshotId());
- Assert.assertEquals(
- "Added files flag must match", expected.hasAddedFiles(), actual.hasAddedFiles());
- Assert.assertEquals(
- "Added files count must match", expected.addedFilesCount(), actual.addedFilesCount());
- Assert.assertEquals(
- "Added rows count must match", expected.addedRowsCount(), actual.addedRowsCount());
- Assert.assertEquals(
- "Existing files flag must match", expected.hasExistingFiles(), actual.hasExistingFiles());
- Assert.assertEquals(
- "Existing files count must match",
- expected.existingFilesCount(),
- actual.existingFilesCount());
- Assert.assertEquals(
- "Existing rows count must match", expected.existingRowsCount(), actual.existingRowsCount());
- Assert.assertEquals(
- "Deleted files flag must match", expected.hasDeletedFiles(), actual.hasDeletedFiles());
- Assert.assertEquals(
- "Deleted files count must match", expected.deletedFilesCount(), actual.deletedFilesCount());
- Assert.assertEquals(
- "Deleted rows count must match", expected.deletedRowsCount(), actual.deletedRowsCount());
+ assertThat(expected != null && actual != null).as("Should not be null.").isTrue();
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415418170
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -483,7 +473,7 @@ private static void assertArrayValues(
private static void assertMapValues(
Types.MapType mapType, LogicalType type, Map<?, ?> expected, MapData actual) {
- Assert.assertEquals("map size should be equal", expected.size(), actual.size());
+ assertThat(actual.size()).as("map size should be equal").isEqualTo(expected.size());
Review Comment:
this should use `hasSameSizeAs(...)`
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1416542497
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,37 +51,28 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
Review Comment:
Ok, will hold off the changes for now
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1862488848
@cgpoh https://github.com/apache/iceberg/pull/9161 was just merged, can you please rebase 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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1863682637
> @cgpoh #9161 was just merged, can you please rebase this PR?
@nastra Thanks, will do that.
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434965909
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,41 +53,36 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
+ MiniFlinkClusterExtension.createWithClassloaderCheckDisabled();
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();
+ @TempDir Path temporaryDirectory;
- @Rule
- public final HadoopCatalogResource catalogResource =
- new HadoopCatalogResource(TEMPORARY_FOLDER, TestFixtures.DATABASE, TestFixtures.TABLE);
+ @RegisterExtension
+ public static final HadoopCatalogExtension catalogExtension =
+ new HadoopCatalogExtension(TestFixtures.DATABASE, TestFixtures.TABLE);
- // parametrized variables
protected final FileFormat fileFormat;
Review Comment:
```suggestion
@Parameter protected FileFormat fileFormat;
```
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434967495
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestIcebergSourceBoundedSql.java:
##########
@@ -26,26 +26,27 @@
import org.apache.flink.table.api.TableEnvironment;
import org.apache.flink.table.api.config.TableConfigOptions;
import org.apache.flink.types.Row;
+import org.apache.iceberg.FileFormat;
import org.apache.iceberg.Schema;
import org.apache.iceberg.expressions.Expression;
import org.apache.iceberg.flink.FlinkConfigOptions;
-import org.junit.Before;
+import org.junit.jupiter.api.BeforeEach;
public class TestIcebergSourceBoundedSql extends TestIcebergSourceBounded {
private volatile TableEnvironment tEnv;
- public TestIcebergSourceBoundedSql(String fileFormat) {
+ public TestIcebergSourceBoundedSql(FileFormat fileFormat) {
Review Comment:
constructor can be removed
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434965644
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,41 +53,36 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
+ MiniFlinkClusterExtension.createWithClassloaderCheckDisabled();
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();
+ @TempDir Path temporaryDirectory;
- @Rule
- public final HadoopCatalogResource catalogResource =
- new HadoopCatalogResource(TEMPORARY_FOLDER, TestFixtures.DATABASE, TestFixtures.TABLE);
+ @RegisterExtension
+ public static final HadoopCatalogExtension catalogExtension =
+ new HadoopCatalogExtension(TestFixtures.DATABASE, TestFixtures.TABLE);
- // parametrized variables
protected final FileFormat fileFormat;
- @Parameterized.Parameters(name = "format={0}")
- public static Object[] parameters() {
- return new Object[] {"avro", "parquet", "orc"};
+ @Parameters(name = "format={0}")
+ public static Collection<FileFormat> fileFormat() {
+ return Arrays.asList(FileFormat.AVRO, FileFormat.PARQUET, FileFormat.ORC);
}
- TestFlinkScan(String fileFormat) {
- this.fileFormat = FileFormat.fromString(fileFormat);
+ TestFlinkScan(FileFormat fileFormat) {
Review Comment:
the constructor needs to be removed because the file format will be set by the parameterized environment
--
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] Flink 1.17: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1878844200
> @cgpoh can you please port these changes to other Flink versions?
Sure. Will try to port these changes next week.
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1411517558
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.io.TempDir;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
+ protected final @TempDir Path temporaryFolder;
Review Comment:
Thanks for the catch! Will remove the annotation.
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+import org.junit.jupiter.api.io.TempDir;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
+ protected final @TempDir Path temporaryFolder;
+ protected final String database;
+ protected final String tableName;
+
+ protected Catalog catalog;
+ protected CatalogLoader catalogLoader;
+ protected String warehouse;
+ protected TableLoader tableLoader;
+
+ public HadoopCatResource(@TempDir Path temporaryFolder, String database, String tableName) {
Review Comment:
Thanks for the catch! Will remove the annotation.
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1410768638
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkInputFormat.java:
##########
@@ -33,24 +33,20 @@
import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.catalog.TableIdentifier;
-import org.apache.iceberg.data.GenericAppenderHelper;
+import org.apache.iceberg.data.GenAppenderHelper;
import org.apache.iceberg.data.RandomGenericData;
import org.apache.iceberg.data.Record;
import org.apache.iceberg.flink.FlinkSchemaUtil;
import org.apache.iceberg.flink.TestFixtures;
import org.apache.iceberg.flink.TestHelpers;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
-import org.junit.Assume;
-import org.junit.Test;
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
/** Test {@link FlinkInputFormat}. */
-public class TestFlinkInputFormat extends TestFlinkSource {
-
- public TestFlinkInputFormat(String fileFormat) {
- super(fileFormat);
- }
-
+public class TestFlinkInputFormat extends TestFlinkSrc {
Review Comment:
How many tests inherit from the `TestFlinkSource`?
Would it be too much to review in one run? Or would it be easier to move all of them at once.
If we move all of them at once, we would not need `TestFlinkSrc`, and we can keep, and migrate `TestFlinkSource` instead
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1411525318
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkInputFormat.java:
##########
@@ -33,24 +33,20 @@
import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.catalog.TableIdentifier;
-import org.apache.iceberg.data.GenericAppenderHelper;
+import org.apache.iceberg.data.GenAppenderHelper;
import org.apache.iceberg.data.RandomGenericData;
import org.apache.iceberg.data.Record;
import org.apache.iceberg.flink.FlinkSchemaUtil;
import org.apache.iceberg.flink.TestFixtures;
import org.apache.iceberg.flink.TestHelpers;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
-import org.junit.Assume;
-import org.junit.Test;
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
/** Test {@link FlinkInputFormat}. */
-public class TestFlinkInputFormat extends TestFlinkSource {
-
- public TestFlinkInputFormat(String fileFormat) {
- super(fileFormat);
- }
-
+public class TestFlinkInputFormat extends TestFlinkSrc {
Review Comment:
I can see the following classes impacted by `TestFlinkSource`:
- TestFlinkScanSql
In `TestFlinkScan`, there's `HadoopCatalogResource` which is used by quite a number of unit tests. I had renamed `HadoopCatalogResource` to `HadoopCatResource` in this commit and migrate the impacted classes listed above.
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkSrc.java:
##########
@@ -0,0 +1,89 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink.source;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.flink.table.api.TableColumn;
+import org.apache.flink.table.api.TableSchema;
+import org.apache.flink.types.Row;
+import org.apache.iceberg.expressions.Expression;
+import org.apache.iceberg.flink.FlinkSchemaUtil;
+import org.apache.iceberg.flink.TestFixtures;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+
+public abstract class TestFlinkSrc extends TestScan {
Review Comment:
I can see the following classes impacted by `TestFlinkScan`:
- TestIcebergSourceBounded
- TestIcebergSourceBoundedSql
In `TestFlinkScan`, there's `HadoopCatalogResource` which is used by quite a number of unit tests. I had renamed `HadoopCatalogResource` to `HadoopCatResource` in this commit and migrate the impacted classes listed 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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415409451
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -309,8 +307,8 @@ public static void assertEquals(Schema schema, List<GenericData.Record> records,
public static void assertEquals(Schema schema, GenericData.Record record, Row row) {
List<Types.NestedField> fields = schema.asStruct().fields();
- Assert.assertEquals(fields.size(), record.getSchema().getFields().size());
- Assert.assertEquals(fields.size(), row.getArity());
+ assertThat(fields.size()).isEqualTo(record.getSchema().getFields().size());
+ assertThat(fields.size()).isEqualTo(row.getArity());
Review Comment:
```suggestion
assertThat(fields).hasSize(row.getArity());
```
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -338,81 +336,73 @@ private static void assertAvroEquals(
return;
}
- Assert.assertTrue(
- "expected and actual should be both null or not null", expected != null && actual != null);
+ assertThat(expected != null && actual != null)
+ .as("expected and actual should be both null or not null")
+ .isTrue();
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415398716
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,37 +51,28 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
Review Comment:
I would hold off on changing any parameterized tests until we have a JUnit5-equivalent that allows parameterization at the class level (which is being handled by https://github.com/apache/iceberg/pull/9161)
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1432508266
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -200,20 +199,19 @@ private void validateIdentityPartitionProjections(
for (int i = 0; i < projectedFields.size(); i++) {
String name = projectedFields.get(i);
- Assert.assertEquals(
- "Projected field " + name + " should match",
- inputRecord.getField(name),
- actualRecord.getField(i));
+ Assertions.assertThat(inputRecord.getField(name))
Review Comment:
it's ok to use static imports for this
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434965009
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkInputFormat.java:
##########
@@ -33,21 +33,20 @@
import org.apache.iceberg.Schema;
import org.apache.iceberg.Table;
import org.apache.iceberg.catalog.TableIdentifier;
-import org.apache.iceberg.data.GenericAppenderHelper;
+import org.apache.iceberg.data.GenAppenderHelper;
import org.apache.iceberg.data.RandomGenericData;
import org.apache.iceberg.data.Record;
import org.apache.iceberg.flink.FlinkSchemaUtil;
import org.apache.iceberg.flink.TestFixtures;
import org.apache.iceberg.flink.TestHelpers;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
-import org.junit.Assume;
-import org.junit.Test;
+import org.junit.jupiter.api.Assumptions;
+import org.junit.jupiter.api.TestTemplate;
/** Test {@link FlinkInputFormat}. */
public class TestFlinkInputFormat extends TestFlinkSource {
-
- public TestFlinkInputFormat(String fileFormat) {
+ public TestFlinkInputFormat(FileFormat fileFormat) {
Review Comment:
all of the constructors in the subclasses can be removed
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1434962899
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkInputFormat.java:
##########
@@ -140,9 +140,9 @@ public void testBasicProjection() throws IOException {
TestHelpers.assertRows(result, expected);
}
- @Test
+ @TestTemplate
public void testReadPartitionColumn() throws Exception {
- Assume.assumeTrue("Temporary skip ORC", FileFormat.ORC != fileFormat);
+ Assumptions.assumeTrue(FileFormat.ORC != fileFormat, "Temporary skip ORC");
Review Comment:
please use Assumptions from AssertJ: `assumeThat(fileFormat).as("Temporary skip ORC").isNotEqualTo(FileFormat.ORC)
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#issuecomment-1878512144
> > Do we have a way to check if the new tests were running on the CI? With the correct parameters?
>
> I don't think so, but I was verifying them all locally
In this case, I am find with these changes.
In the next PR we should port this changes to 1.18, and 1.16 too
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1416540362
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
Review Comment:
Thanks. I didn’t know about `HadoopCatalogExtension`, will use that instead
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
Review Comment:
Thanks. I didn’t know about `HadoopCatalogExtension`, will use that instead
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1418227381
##########
data/src/test/java/org/apache/iceberg/data/GenAppenderHelper.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AppendFiles;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.assertj.core.api.Assertions;
+
+/** Helper for appending {@link DataFile} to a table or appending {@link Record}s to a table. */
+public class GenAppenderHelper {
Review Comment:
@pvary , `GenericAppenderHelper` is also used by iceberg-mr package. If I migrate `GenericAppenderHelper`, I have to migrate the test in iceberg-mr package too.
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1416542177
##########
data/src/test/java/org/apache/iceberg/data/GenAppenderHelper.java:
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.data;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import java.util.List;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.AppendFiles;
+import org.apache.iceberg.DataFile;
+import org.apache.iceberg.DataFiles;
+import org.apache.iceberg.FileFormat;
+import org.apache.iceberg.Files;
+import org.apache.iceberg.SnapshotRef;
+import org.apache.iceberg.StructLike;
+import org.apache.iceberg.Table;
+import org.apache.iceberg.io.FileAppender;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.assertj.core.api.Assertions;
+
+/** Helper for appending {@link DataFile} to a table or appending {@link Record}s to a table. */
+public class GenAppenderHelper {
Review Comment:
Let me see whether we can do that.
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "cgpoh (via GitHub)" <gi...@apache.org>.
cgpoh commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1418251955
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -483,7 +473,7 @@ private static void assertArrayValues(
private static void assertMapValues(
Types.MapType mapType, LogicalType type, Map<?, ?> expected, MapData actual) {
- Assert.assertEquals("map size should be equal", expected.size(), actual.size());
+ assertThat(actual.size()).as("map size should be equal").isEqualTo(expected.size());
Review Comment:
@nastra , I have problem using `hasSameSizeAs(...)` here as MapData is not extending from `Collection`, `assertThat` has difficulty inferring that `actual` can be enumerated.
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415518602
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/HadoopCatResource.java:
##########
@@ -0,0 +1,93 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.flink;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Path;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.iceberg.CatalogProperties;
+import org.apache.iceberg.catalog.Catalog;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.hadoop.HadoopCatalog;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.jupiter.api.extension.AfterEachCallback;
+import org.junit.jupiter.api.extension.BeforeEachCallback;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+public class HadoopCatResource implements BeforeEachCallback, AfterEachCallback {
Review Comment:
yes I think we can just use `HadoopCatalogExtension`, thus we can remove `HadoopCatResource`
--
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] chore: junit 5 migration for TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1415401117
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -159,9 +159,9 @@ public static void assertRowData(
return;
}
- Assert.assertTrue(
- "expected Record and actual RowData should be both null or not null",
- expectedRecord != null && actualRowData != null);
+ assertThat(expectedRecord != null && actualRowData != null)
Review Comment:
this should be `assertThat(expectedRecord).isNotNull()` and then `assertThat(actualRowData).isNotNull()`.
We want to avoid using isTrue/isFalse as much as possible, because it's more difficult to debug when the assertion fails
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "rodmeneses (via GitHub)" <gi...@apache.org>.
rodmeneses commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1420810926
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -523,89 +510,103 @@ public static void assertEquals(ManifestFile expected, ManifestFile actual) {
if (expected == actual) {
return;
}
- Assert.assertTrue("Should not be null.", expected != null && actual != null);
- Assert.assertEquals("Path must match", expected.path(), actual.path());
- Assert.assertEquals("Length must match", expected.length(), actual.length());
- Assert.assertEquals("Spec id must match", expected.partitionSpecId(), actual.partitionSpecId());
- Assert.assertEquals("ManifestContent must match", expected.content(), actual.content());
- Assert.assertEquals(
- "SequenceNumber must match", expected.sequenceNumber(), actual.sequenceNumber());
- Assert.assertEquals(
- "MinSequenceNumber must match", expected.minSequenceNumber(), actual.minSequenceNumber());
- Assert.assertEquals("Snapshot id must match", expected.snapshotId(), actual.snapshotId());
- Assert.assertEquals(
- "Added files flag must match", expected.hasAddedFiles(), actual.hasAddedFiles());
- Assert.assertEquals(
- "Added files count must match", expected.addedFilesCount(), actual.addedFilesCount());
- Assert.assertEquals(
- "Added rows count must match", expected.addedRowsCount(), actual.addedRowsCount());
- Assert.assertEquals(
- "Existing files flag must match", expected.hasExistingFiles(), actual.hasExistingFiles());
- Assert.assertEquals(
- "Existing files count must match",
- expected.existingFilesCount(),
- actual.existingFilesCount());
- Assert.assertEquals(
- "Existing rows count must match", expected.existingRowsCount(), actual.existingRowsCount());
- Assert.assertEquals(
- "Deleted files flag must match", expected.hasDeletedFiles(), actual.hasDeletedFiles());
- Assert.assertEquals(
- "Deleted files count must match", expected.deletedFilesCount(), actual.deletedFilesCount());
- Assert.assertEquals(
- "Deleted rows count must match", expected.deletedRowsCount(), actual.deletedRowsCount());
+ assertThat(expected).isNotNull();
+ assertThat(actual).isNotNull();
+ assertThat(actual.path()).as("Path must match").isEqualTo(expected.path());
+ assertThat(actual.length()).as("Length must match").isEqualTo(expected.length());
+ assertThat(actual.partitionSpecId())
+ .as("Spec id must match")
+ .isEqualTo(expected.partitionSpecId());
+ assertThat(actual.content()).as("ManifestContent must match").isEqualTo(expected.content());
+ assertThat(actual.sequenceNumber())
+ .as("SequenceNumber must match")
+ .isEqualTo(expected.sequenceNumber());
+ assertThat(actual.minSequenceNumber())
+ .as("MinSequenceNumber must match")
+ .isEqualTo(expected.minSequenceNumber());
+ assertThat(actual.snapshotId()).as("Snapshot id must match").isEqualTo(expected.snapshotId());
+ assertThat(actual.hasAddedFiles())
+ .as("Added files flag must match")
+ .isEqualTo(expected.hasAddedFiles());
+ assertThat(actual.addedFilesCount())
+ .as("Added files count must match")
+ .isEqualTo(expected.addedFilesCount());
+ assertThat(actual.addedRowsCount())
+ .as("Added rows count must match")
+ .isEqualTo(expected.addedRowsCount());
+ assertThat(actual.hasExistingFiles())
+ .as("Existing files flag must match")
+ .isEqualTo(expected.hasExistingFiles());
+ assertThat(actual.existingFilesCount())
+ .as("Existing files count must match")
+ .isEqualTo(expected.existingFilesCount());
+ assertThat(actual.existingRowsCount())
+ .as("Existing rows count must match")
+ .isEqualTo(expected.existingRowsCount());
+ assertThat(actual.hasDeletedFiles())
+ .as("Deleted files flag must match")
+ .isEqualTo(expected.hasDeletedFiles());
+ assertThat(actual.deletedFilesCount())
+ .as("Deleted files count must match")
+ .isEqualTo(expected.deletedFilesCount());
+ assertThat(actual.deletedRowsCount())
+ .as("Deleted rows count must match")
+ .isEqualTo(expected.deletedRowsCount());
List<ManifestFile.PartitionFieldSummary> expectedSummaries = expected.partitions();
List<ManifestFile.PartitionFieldSummary> actualSummaries = actual.partitions();
- Assert.assertEquals(
- "PartitionFieldSummary size does not match",
- expectedSummaries.size(),
- actualSummaries.size());
+ assertThat(actualSummaries)
+ .as("PartitionFieldSummary size does not match")
+ .hasSameSizeAs(expectedSummaries.size());
Review Comment:
I dont think this is the right way to use `hasSameSizeAs` assertions.
the parameter of
```java
.hasSameSizeAs
```
shouldn't be a number, but instead another collection.
Please make changes accordingly to the rest of the codebase
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "rodmeneses (via GitHub)" <gi...@apache.org>.
rodmeneses commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1420797428
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/TestHelpers.java:
##########
@@ -193,109 +192,106 @@ private static void assertEquals(
return;
}
- Assert.assertTrue(
- "expected and actual should be both null or not null", expected != null && actual != null);
+ assertThat(expected).isNotNull();
+ assertThat(actual).isNotNull();
Review Comment:
from the previous assertion I can read:
```"expected and actual should be both null or not null```
however, this block
```java
assertThat(expected).isNotNull();
assertThat(actual).isNotNull();
```
is only asserting that both of them are not null
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1432507299
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,41 +53,36 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
+ MiniFlinkClusterExtension.createWithClassloaderCheckDisabled();
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();
+ @TempDir Path temporaryDirectory;
- @Rule
- public final HadoopCatalogResource catalogResource =
- new HadoopCatalogResource(TEMPORARY_FOLDER, TestFixtures.DATABASE, TestFixtures.TABLE);
+ @RegisterExtension
+ public static final HadoopCatalogExtension catalogExtension =
+ new HadoopCatalogExtension(TestFixtures.DATABASE, TestFixtures.TABLE);
- // parametrized variables
Review Comment:
should this be annotated with `@Parameter`?
--
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] Flink: Create JUnit5 version of TestFlinkScan [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9185:
URL: https://github.com/apache/iceberg/pull/9185#discussion_r1432507642
##########
flink/v1.17/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkScan.java:
##########
@@ -49,41 +53,36 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.DateTimeUtil;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public abstract class TestFlinkScan {
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.api.io.TempDir;
- @ClassRule
- public static final MiniClusterWithClientResource MINI_CLUSTER_RESOURCE =
- MiniClusterResource.createWithClassloaderCheckDisabled();
+@ExtendWith(ParameterizedTestExtension.class)
+public abstract class TestFlinkScan {
+ @RegisterExtension
+ public static MiniClusterExtension miniClusterResource =
+ MiniFlinkClusterExtension.createWithClassloaderCheckDisabled();
- @ClassRule public static final TemporaryFolder TEMPORARY_FOLDER = new TemporaryFolder();
+ @TempDir Path temporaryDirectory;
Review Comment:
```suggestion
@TempDir protected Path temp;
```
--
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