You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "coded9 (via GitHub)" <gi...@apache.org> on 2023/10/29 06:17:52 UTC
[PR] Flink v1.15 : Remove usage of AssertHelpers [iceberg]
coded9 opened a new pull request, #8945:
URL: https://github.com/apache/iceberg/pull/8945
(no comment)
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "coded9 (via GitHub)" <gi...@apache.org>.
coded9 commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375963552
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkSourceConfig.java:
##########
@@ -31,14 +31,13 @@ public class TestFlinkSourceConfig extends TestFlinkTableSource {
@Test
public void testFlinkSessionConfig() {
getTableEnv().getConfig().set(FlinkReadOptions.STREAMING_OPTION, true);
- AssertHelpers.assertThrows(
- "Should throw exception because of cannot set snapshot-id option for streaming reader",
- IllegalArgumentException.class,
- "Cannot set as-of-timestamp option for streaming reader",
- () -> {
- sql("SELECT * FROM %s /*+ OPTIONS('as-of-timestamp'='1')*/", TABLE);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ sql("SELECT * FROM %s /*+ OPTIONS('as-of-timestamp'='1')*/", TABLE);
+ })
+ .as("Should throw exception because of cannot set snapshot-id option for streaming reader")
Review Comment:
Addressed for most of the cases
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkTableSource.java:
##########
@@ -611,44 +608,38 @@ public void testFilterPushDown2Literal() {
public void testSqlParseError() {
String sqlParseErrorEqual =
String.format("SELECT * FROM %s WHERE d = CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorEqual));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorEqual))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorNotEqual =
String.format("SELECT * FROM %s WHERE d <> CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorNotEqual));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorNotEqual))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorGT =
String.format("SELECT * FROM %s WHERE d > CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorGT));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorGT))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorLT =
String.format("SELECT * FROM %s WHERE d < CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorLT));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorLT))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorGTE =
String.format("SELECT * FROM %s WHERE d >= CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorGTE));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorGTE))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorLTE =
String.format("SELECT * FROM %s WHERE d <= CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorLTE));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorLTE))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
Review Comment:
Addressed
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375970052
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -197,15 +197,12 @@ public void testJobHashDistributionMode() {
.updateProperties()
.set(TableProperties.WRITE_DISTRIBUTION_MODE, DistributionMode.HASH.modeName())
.commit();
-
- AssertHelpers.assertThrows(
- "Does not support range distribution-mode now.",
- IllegalArgumentException.class,
- "Flink does not support 'range' write distribution mode now.",
- () -> {
- testWriteRow(null, DistributionMode.RANGE);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
Review Comment:
the {} aren't needed here and 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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375971134
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,18 +347,14 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.tableLoader(tableLoader)
.writeParallelism(parallelism)
.setAll(newProps);
-
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
Review Comment:
```
Assertions.assertThatThrownBy(builder::append)
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid distribution mode: UNRECOGNIZED");
```
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1376215669
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSinkV2Base.java:
##########
@@ -231,20 +231,20 @@ protected void testChangeLogOnIdKey(String branch) throws Exception {
ImmutableList.of(record(1, "ddd"), record(2, "ddd")));
if (partitioned && writeDistributionMode.equals(TableProperties.WRITE_DISTRIBUTION_MODE_HASH)) {
- AssertHelpers.assertThrows(
- "Should be error because equality field columns don't include all partition keys",
- IllegalStateException.class,
- "should be included in equality fields",
- () -> {
- testChangeLogs(
- ImmutableList.of("id"),
- row -> row.getField(ROW_ID_POS),
- false,
- elementsPerCheckpoint,
- expectedRecords,
- branch);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
Review Comment:
the {} are not needed and 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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375769799
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/enumerator/TestContinuousSplitPlannerImpl.java:
##########
@@ -337,12 +337,10 @@ public void testIncrementalFromSnapshotIdWithEmptyTable() throws Exception {
ContinuousSplitPlannerImpl splitPlanner =
new ContinuousSplitPlannerImpl(
tableResource.tableLoader().clone(), scanContextWithInvalidSnapshotId, null);
-
- AssertHelpers.assertThrows(
- "Should detect invalid starting snapshot id",
- IllegalArgumentException.class,
- "Start snapshot id not found in history: 1",
- () -> splitPlanner.planSplits(null));
+ Assertions.assertThatThrownBy(() -> splitPlanner.planSplits(null))
+ .as("Should detect invalid starting snapshot id")
Review Comment:
not needed because it's implicit in the error msg
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375972716
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,18 +347,14 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.tableLoader(tableLoader)
.writeParallelism(parallelism)
.setAll(newProps);
-
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
Review Comment:
this is then also in-line with the Flink 1.17 test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Flink v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1376216030
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSinkV2Base.java:
##########
@@ -278,20 +278,18 @@ protected void testUpsertOnIdKey(String branch) throws Exception {
expectedRecords,
branch);
} else {
- AssertHelpers.assertThrows(
- "Should be error because equality field columns don't include all partition keys",
- IllegalStateException.class,
- "should be included in equality fields",
- () -> {
- testChangeLogs(
- ImmutableList.of("id"),
- row -> row.getField(ROW_ID_POS),
- true,
- elementsPerCheckpoint,
- expectedRecords,
- branch);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
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] Flink v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "coded9 (via GitHub)" <gi...@apache.org>.
coded9 commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1377358392
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestStreamScanSql.java:
##########
@@ -219,15 +219,13 @@ public void testConsumeFilesWithBranch() throws Exception {
Row row1 = Row.of(1, "aaa", "2021-01-01");
Row row2 = Row.of(2, "bbb", "2021-01-01");
insertRows(table, row1, row2);
-
- AssertHelpers.assertThrows(
- "Cannot scan table using ref for stream yet",
- IllegalArgumentException.class,
- "Cannot scan table using ref",
- () ->
- exec(
- "SELECT * FROM %s /*+ OPTIONS('streaming'='true', 'monitor-interval'='1s', 'branch'='b1')*/",
- TABLE));
+ Assertions.assertThatThrownBy(
+ () ->
+ exec(
+ "SELECT * FROM %s /*+ OPTIONS('streaming'='true', 'monitor-interval'='1s', 'branch'='b1')*/",
+ TABLE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot scan table using ref");
Review Comment:
Changed the message
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375969223
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestIcebergConnector.java:
##########
@@ -261,11 +261,11 @@ public void testCatalogDatabaseConflictWithFlinkDatabase() {
try {
testCreateConnectorTable();
// Ensure that the table was created under the specific database.
- AssertHelpers.assertThrows(
- "Table should already exists",
- org.apache.flink.table.api.TableException.class,
- "Could not execute CreateTable in path",
- () -> sql("CREATE TABLE `default_catalog`.`%s`.`%s`", databaseName(), TABLE_NAME));
+ Assertions.assertThatThrownBy(
+ () -> sql("CREATE TABLE `default_catalog`.`%s`.`%s`", databaseName(), TABLE_NAME))
+ .as("Table should already exists")
Review Comment:
```suggestion
.as("Table should already exist")
```
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "coded9 (via GitHub)" <gi...@apache.org>.
coded9 commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375962320
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTablePartitions.java:
##########
@@ -90,10 +90,9 @@ public void testListPartitionsWithUnpartitionedTable() {
ObjectPath objectPath = new ObjectPath(DATABASE, tableName);
FlinkCatalog flinkCatalog = (FlinkCatalog) getTableEnv().getCatalog(catalogName).get();
- AssertHelpers.assertThrows(
- "Should not list partitions for unpartitioned table.",
- TableNotPartitionedException.class,
- () -> flinkCatalog.listPartitions(objectPath));
+ Assertions.assertThatThrownBy(() -> flinkCatalog.listPartitions(objectPath))
+ .as("Should not list partitions for unpartitioned table.")
+ .isInstanceOf(TableNotPartitionedException.class);
Review Comment:
Added the `.hasMessageContaining` check
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "fivetran-ashokborra (via GitHub)" <gi...@apache.org>.
fivetran-ashokborra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375960476
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTablePartitions.java:
##########
@@ -90,10 +90,9 @@ public void testListPartitionsWithUnpartitionedTable() {
ObjectPath objectPath = new ObjectPath(DATABASE, tableName);
FlinkCatalog flinkCatalog = (FlinkCatalog) getTableEnv().getCatalog(catalogName).get();
- AssertHelpers.assertThrows(
- "Should not list partitions for unpartitioned table.",
- TableNotPartitionedException.class,
- () -> flinkCatalog.listPartitions(objectPath));
+ Assertions.assertThatThrownBy(() -> flinkCatalog.listPartitions(objectPath))
+ .as("Should not list partitions for unpartitioned table.")
+ .isInstanceOf(TableNotPartitionedException.class);
Review Comment:
Added the `.hasMessageContaining` check
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -197,15 +197,13 @@ public void testJobHashDistributionMode() {
.updateProperties()
.set(TableProperties.WRITE_DISTRIBUTION_MODE, DistributionMode.HASH.modeName())
.commit();
-
- AssertHelpers.assertThrows(
- "Does not support range distribution-mode now.",
- IllegalArgumentException.class,
- "Flink does not support 'range' write distribution mode now.",
- () -> {
- testWriteRow(null, DistributionMode.RANGE);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ testWriteRow(null, DistributionMode.RANGE);
+ })
+ .as("Does not support range distribution-mode now.")
Review Comment:
Addressed
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,18 +348,15 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.tableLoader(tableLoader)
.writeParallelism(parallelism)
.setAll(newProps);
-
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ builder.append();
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
+ })
+ .as("Should fail with invalid distribution mode.")
Review Comment:
Addressed
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1376218462
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestStreamScanSql.java:
##########
@@ -219,15 +219,13 @@ public void testConsumeFilesWithBranch() throws Exception {
Row row1 = Row.of(1, "aaa", "2021-01-01");
Row row2 = Row.of(2, "bbb", "2021-01-01");
insertRows(table, row1, row2);
-
- AssertHelpers.assertThrows(
- "Cannot scan table using ref for stream yet",
- IllegalArgumentException.class,
- "Cannot scan table using ref",
- () ->
- exec(
- "SELECT * FROM %s /*+ OPTIONS('streaming'='true', 'monitor-interval'='1s', 'branch'='b1')*/",
- TABLE));
+ Assertions.assertThatThrownBy(
+ () ->
+ exec(
+ "SELECT * FROM %s /*+ OPTIONS('streaming'='true', 'monitor-interval'='1s', 'branch'='b1')*/",
+ TABLE))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot scan table using ref");
Review Comment:
should be aligned with https://github.com/apache/iceberg/pull/7481/files#diff-9c8a51c635b89bc826ca641580135b1f954cd8bbf9f1ee5443a2f182b1d9f311R229
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestStreamScanSql.java:
##########
@@ -306,15 +304,13 @@ public void testConsumeFromStartTag() throws Exception {
assertRows(ImmutableList.of(row7), iterator);
}
result.getJobClient().ifPresent(JobClient::cancel);
-
- AssertHelpers.assertThrows(
- "START_SNAPSHOT_ID and START_TAG cannot both be set.",
- IllegalArgumentException.class,
- "START_SNAPSHOT_ID and START_TAG cannot both be set.",
- () ->
- exec(
- "SELECT * FROM %s /*+ OPTIONS('streaming'='true', 'monitor-interval'='1s', 'start-tag'='%s', "
- + "'start-snapshot-id'='%d' )*/",
- TABLE, tagName, startSnapshotId));
+ Assertions.assertThatThrownBy(
+ () ->
+ exec(
+ "SELECT * FROM %s /*+ OPTIONS('streaming'='true', 'monitor-interval'='1s', 'start-tag'='%s', "
+ + "'start-snapshot-id'='%d' )*/",
+ TABLE, tagName, startSnapshotId))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("START_SNAPSHOT_ID and START_TAG cannot both be set.");
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] Flink v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1376219994
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/enumerator/TestContinuousSplitPlannerImpl.java:
##########
@@ -337,12 +337,10 @@ public void testIncrementalFromSnapshotIdWithEmptyTable() throws Exception {
ContinuousSplitPlannerImpl splitPlanner =
new ContinuousSplitPlannerImpl(
tableResource.tableLoader().clone(), scanContextWithInvalidSnapshotId, null);
-
- AssertHelpers.assertThrows(
- "Should detect invalid starting snapshot id",
- IllegalArgumentException.class,
- "Start snapshot id not found in history: 1",
- () -> splitPlanner.planSplits(null));
+ Assertions.assertThatThrownBy(() -> splitPlanner.planSplits(null))
+ .as("Should detect invalid starting snapshot id")
Review Comment:
unresolving this as it hasn't been addressed
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/enumerator/TestContinuousSplitPlannerImpl.java:
##########
@@ -365,12 +363,10 @@ public void testIncrementalFromSnapshotIdWithInvalidIds() throws Exception {
ContinuousSplitPlannerImpl splitPlanner =
new ContinuousSplitPlannerImpl(
tableResource.tableLoader().clone(), scanContextWithInvalidSnapshotId, null);
-
- AssertHelpers.assertThrows(
- "Should detect invalid starting snapshot id",
- IllegalArgumentException.class,
- "Start snapshot id not found in history: " + invalidSnapshotId,
- () -> splitPlanner.planSplits(null));
+ Assertions.assertThatThrownBy(() -> splitPlanner.planSplits(null))
+ .as("Should detect invalid starting snapshot id")
Review Comment:
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375764723
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTablePartitions.java:
##########
@@ -90,10 +90,9 @@ public void testListPartitionsWithUnpartitionedTable() {
ObjectPath objectPath = new ObjectPath(DATABASE, tableName);
FlinkCatalog flinkCatalog = (FlinkCatalog) getTableEnv().getCatalog(catalogName).get();
- AssertHelpers.assertThrows(
- "Should not list partitions for unpartitioned table.",
- TableNotPartitionedException.class,
- () -> flinkCatalog.listPartitions(objectPath));
+ Assertions.assertThatThrownBy(() -> flinkCatalog.listPartitions(objectPath))
+ .as("Should not list partitions for unpartitioned table.")
+ .isInstanceOf(TableNotPartitionedException.class);
Review Comment:
this needs a `.hasMessageXyz()` check
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "coded9 (via GitHub)" <gi...@apache.org>.
coded9 commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375963102
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -197,15 +197,13 @@ public void testJobHashDistributionMode() {
.updateProperties()
.set(TableProperties.WRITE_DISTRIBUTION_MODE, DistributionMode.HASH.modeName())
.commit();
-
- AssertHelpers.assertThrows(
- "Does not support range distribution-mode now.",
- IllegalArgumentException.class,
- "Flink does not support 'range' write distribution mode now.",
- () -> {
- testWriteRow(null, DistributionMode.RANGE);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ testWriteRow(null, DistributionMode.RANGE);
+ })
+ .as("Does not support range distribution-mode now.")
Review Comment:
Addressed
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1376219315
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestStreamingMonitorFunction.java:
##########
@@ -254,30 +254,24 @@ public void testInvalidMaxPlanningSnapshotCount() {
.monitorInterval(Duration.ofMillis(100))
.maxPlanningSnapshotCount(0)
.build();
-
- AssertHelpers.assertThrows(
- "Should throw exception because of invalid config",
- IllegalArgumentException.class,
- "must be greater than zero",
- () -> {
- createFunction(scanContext1);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
Review Comment:
same as above
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestStreamingMonitorFunction.java:
##########
@@ -254,30 +254,24 @@ public void testInvalidMaxPlanningSnapshotCount() {
.monitorInterval(Duration.ofMillis(100))
.maxPlanningSnapshotCount(0)
.build();
-
- AssertHelpers.assertThrows(
- "Should throw exception because of invalid config",
- IllegalArgumentException.class,
- "must be greater than zero",
- () -> {
- createFunction(scanContext1);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ createFunction(scanContext1);
+ })
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("The max-planning-snapshot-count must be greater than zero");
ScanContext scanContext2 =
ScanContext.builder()
.monitorInterval(Duration.ofMillis(100))
.maxPlanningSnapshotCount(-10)
.build();
-
- AssertHelpers.assertThrows(
- "Should throw exception because of invalid config",
- IllegalArgumentException.class,
- "must be greater than zero",
- () -> {
- createFunction(scanContext2);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
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] Flink v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375768630
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkSourceConfig.java:
##########
@@ -31,14 +31,13 @@ public class TestFlinkSourceConfig extends TestFlinkTableSource {
@Test
public void testFlinkSessionConfig() {
getTableEnv().getConfig().set(FlinkReadOptions.STREAMING_OPTION, true);
- AssertHelpers.assertThrows(
- "Should throw exception because of cannot set snapshot-id option for streaming reader",
- IllegalArgumentException.class,
- "Cannot set as-of-timestamp option for streaming reader",
- () -> {
- sql("SELECT * FROM %s /*+ OPTIONS('as-of-timestamp'='1')*/", TABLE);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ sql("SELECT * FROM %s /*+ OPTIONS('as-of-timestamp'='1')*/", TABLE);
+ })
+ .as("Should throw exception because of cannot set snapshot-id option for streaming reader")
Review Comment:
not needed. Please re-visit all `.as()` usage in this PR. I believe most are not needed
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375765951
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -197,15 +197,13 @@ public void testJobHashDistributionMode() {
.updateProperties()
.set(TableProperties.WRITE_DISTRIBUTION_MODE, DistributionMode.HASH.modeName())
.commit();
-
- AssertHelpers.assertThrows(
- "Does not support range distribution-mode now.",
- IllegalArgumentException.class,
- "Flink does not support 'range' write distribution mode now.",
- () -> {
- testWriteRow(null, DistributionMode.RANGE);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ testWriteRow(null, DistributionMode.RANGE);
+ })
+ .as("Does not support range distribution-mode now.")
Review Comment:
not needed as it's implicit through the expected error msg. The same is true for most other places
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375971899
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -378,18 +371,14 @@ public void testOverrideWriteConfigWithUnknownFileFormat() {
.tableLoader(tableLoader)
.writeParallelism(parallelism)
.setAll(newProps);
-
- AssertHelpers.assertThrows(
- "Should fail with invalid file format.",
- IllegalArgumentException.class,
- "Invalid file format: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
Review Comment:
same as above. the error happens when append is called, not when execute is called
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375769193
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkTableSource.java:
##########
@@ -611,44 +608,38 @@ public void testFilterPushDown2Literal() {
public void testSqlParseError() {
String sqlParseErrorEqual =
String.format("SELECT * FROM %s WHERE d = CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorEqual));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorEqual))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorNotEqual =
String.format("SELECT * FROM %s WHERE d <> CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorNotEqual));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorNotEqual))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorGT =
String.format("SELECT * FROM %s WHERE d > CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorGT));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorGT))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorLT =
String.format("SELECT * FROM %s WHERE d < CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorLT));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorLT))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorGTE =
String.format("SELECT * FROM %s WHERE d >= CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorGTE));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorGTE))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
String sqlParseErrorLTE =
String.format("SELECT * FROM %s WHERE d <= CAST('NaN' AS DOUBLE) ", TABLE_NAME);
- AssertHelpers.assertThrows(
- "The NaN is not supported by flink now. ",
- NumberFormatException.class,
- () -> sql(sqlParseErrorLTE));
+ Assertions.assertThatThrownBy(() -> sql(sqlParseErrorLTE))
+ .as("The NaN is not supported by flink now. ")
+ .isInstanceOf(NumberFormatException.class);
Review Comment:
all of these need to check the underlying error msg. Also `.as()` isn't needed
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "fivetran-ashokborra (via GitHub)" <gi...@apache.org>.
fivetran-ashokborra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375959253
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/enumerator/TestContinuousSplitPlannerImpl.java:
##########
@@ -337,12 +337,10 @@ public void testIncrementalFromSnapshotIdWithEmptyTable() throws Exception {
ContinuousSplitPlannerImpl splitPlanner =
new ContinuousSplitPlannerImpl(
tableResource.tableLoader().clone(), scanContextWithInvalidSnapshotId, null);
-
- AssertHelpers.assertThrows(
- "Should detect invalid starting snapshot id",
- IllegalArgumentException.class,
- "Start snapshot id not found in history: 1",
- () -> splitPlanner.planSplits(null));
+ Assertions.assertThatThrownBy(() -> splitPlanner.planSplits(null))
+ .as("Should detect invalid starting snapshot id")
Review Comment:
Changes made as suggested
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "fivetran-ashokborra (via GitHub)" <gi...@apache.org>.
fivetran-ashokborra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375959947
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java:
##########
@@ -277,12 +275,11 @@ public void testDowngradeTableToFormatV1ThroughTablePropertyFails() throws Excep
Table table = table("tl");
TableOperations ops = ((BaseTable) table).operations();
Assert.assertEquals("should create table using format v2", 2, ops.refresh().formatVersion());
-
- AssertHelpers.assertThrowsRootCause(
- "should fail to downgrade to v1",
- IllegalArgumentException.class,
- "Cannot downgrade v2 table to v1",
- () -> sql("ALTER TABLE tl SET('format-version'='1')"));
+ Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl SET('format-version'='1')"))
+ .as("should fail to downgrade to v1")
Review Comment:
Changes made as suggested
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "coded9 (via GitHub)" <gi...@apache.org>.
coded9 commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375961741
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java:
##########
@@ -277,12 +275,11 @@ public void testDowngradeTableToFormatV1ThroughTablePropertyFails() throws Excep
Table table = table("tl");
TableOperations ops = ((BaseTable) table).operations();
Assert.assertEquals("should create table using format v2", 2, ops.refresh().formatVersion());
-
- AssertHelpers.assertThrowsRootCause(
- "should fail to downgrade to v1",
- IllegalArgumentException.class,
- "Cannot downgrade v2 table to v1",
- () -> sql("ALTER TABLE tl SET('format-version'='1')"));
+ Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl SET('format-version'='1')"))
+ .as("should fail to downgrade to v1")
Review Comment:
Changes made as suggested
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #8945:
URL: https://github.com/apache/iceberg/pull/8945
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375764386
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java:
##########
@@ -277,12 +275,11 @@ public void testDowngradeTableToFormatV1ThroughTablePropertyFails() throws Excep
Table table = table("tl");
TableOperations ops = ((BaseTable) table).operations();
Assert.assertEquals("should create table using format v2", 2, ops.refresh().formatVersion());
-
- AssertHelpers.assertThrowsRootCause(
- "should fail to downgrade to v1",
- IllegalArgumentException.class,
- "Cannot downgrade v2 table to v1",
- () -> sql("ALTER TABLE tl SET('format-version'='1')"));
+ Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl SET('format-version'='1')"))
+ .as("should fail to downgrade to v1")
Review Comment:
I think in most cases we don't need `.as()` because the check is self-explanatory
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "fivetran-ashokborra (via GitHub)" <gi...@apache.org>.
fivetran-ashokborra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375960476
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTablePartitions.java:
##########
@@ -90,10 +90,9 @@ public void testListPartitionsWithUnpartitionedTable() {
ObjectPath objectPath = new ObjectPath(DATABASE, tableName);
FlinkCatalog flinkCatalog = (FlinkCatalog) getTableEnv().getCatalog(catalogName).get();
- AssertHelpers.assertThrows(
- "Should not list partitions for unpartitioned table.",
- TableNotPartitionedException.class,
- () -> flinkCatalog.listPartitions(objectPath));
+ Assertions.assertThatThrownBy(() -> flinkCatalog.listPartitions(objectPath))
+ .as("Should not list partitions for unpartitioned table.")
+ .isInstanceOf(TableNotPartitionedException.class);
Review Comment:
Added the `.hasMessageContaining` check
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,18 +348,15 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.tableLoader(tableLoader)
.writeParallelism(parallelism)
.setAll(newProps);
-
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ builder.append();
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
+ })
+ .as("Should fail with invalid distribution mode.")
Review Comment:
Addressed
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375974519
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSinkV2.java:
##########
@@ -201,19 +201,19 @@ public void testUpsertModeCheck() throws Exception {
.tableSchema(SimpleDataUtil.FLINK_SCHEMA)
.writeParallelism(parallelism)
.upsert(true);
-
- AssertHelpers.assertThrows(
- "Should be error because upsert mode and overwrite mode enable at the same time.",
- IllegalStateException.class,
- "OVERWRITE mode shouldn't be enable",
- () ->
- builder.equalityFieldColumns(ImmutableList.of("id", "data")).overwrite(true).append());
-
- AssertHelpers.assertThrows(
- "Should be error because equality field columns are empty.",
- IllegalStateException.class,
- "Equality field columns shouldn't be empty",
- () -> builder.equalityFieldColumns(ImmutableList.of()).overwrite(false).append());
+ Assertions.assertThatThrownBy(
Review Comment:
please align this with how it's done for Flink 1.17, where it uses `.hasMessage("OVERWRITE mode shouldn't be enable when configuring to use UPSERT data stream.")`
same for all the other places
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "fivetran-ashokborra (via GitHub)" <gi...@apache.org>.
fivetran-ashokborra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375959253
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/enumerator/TestContinuousSplitPlannerImpl.java:
##########
@@ -337,12 +337,10 @@ public void testIncrementalFromSnapshotIdWithEmptyTable() throws Exception {
ContinuousSplitPlannerImpl splitPlanner =
new ContinuousSplitPlannerImpl(
tableResource.tableLoader().clone(), scanContextWithInvalidSnapshotId, null);
-
- AssertHelpers.assertThrows(
- "Should detect invalid starting snapshot id",
- IllegalArgumentException.class,
- "Start snapshot id not found in history: 1",
- () -> splitPlanner.planSplits(null));
+ Assertions.assertThatThrownBy(() -> splitPlanner.planSplits(null))
+ .as("Should detect invalid starting snapshot id")
Review Comment:
Changes made as suggested
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375766136
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -350,18 +348,15 @@ public void testOverrideWriteConfigWithUnknownDistributionMode() {
.tableLoader(tableLoader)
.writeParallelism(parallelism)
.setAll(newProps);
-
- AssertHelpers.assertThrows(
- "Should fail with invalid distribution mode.",
- IllegalArgumentException.class,
- "Invalid distribution mode: UNRECOGNIZED",
- () -> {
- builder.append();
-
- // Execute the program.
- env.execute("Test Iceberg DataStream.");
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ builder.append();
+ // Execute the program.
+ env.execute("Test Iceberg DataStream.");
+ })
+ .as("Should fail with invalid distribution mode.")
Review Comment:
not needed
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "fivetran-ashokborra (via GitHub)" <gi...@apache.org>.
fivetran-ashokborra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375959947
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/TestFlinkCatalogTable.java:
##########
@@ -277,12 +275,11 @@ public void testDowngradeTableToFormatV1ThroughTablePropertyFails() throws Excep
Table table = table("tl");
TableOperations ops = ((BaseTable) table).operations();
Assert.assertEquals("should create table using format v2", 2, ops.refresh().formatVersion());
-
- AssertHelpers.assertThrowsRootCause(
- "should fail to downgrade to v1",
- IllegalArgumentException.class,
- "Cannot downgrade v2 table to v1",
- () -> sql("ALTER TABLE tl SET('format-version'='1')"));
+ Assertions.assertThatThrownBy(() -> sql("ALTER TABLE tl SET('format-version'='1')"))
+ .as("should fail to downgrade to v1")
Review Comment:
Changes made as suggested
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "fivetran-ashokborra (via GitHub)" <gi...@apache.org>.
fivetran-ashokborra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1375960800
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/sink/TestFlinkIcebergSink.java:
##########
@@ -197,15 +197,13 @@ public void testJobHashDistributionMode() {
.updateProperties()
.set(TableProperties.WRITE_DISTRIBUTION_MODE, DistributionMode.HASH.modeName())
.commit();
-
- AssertHelpers.assertThrows(
- "Does not support range distribution-mode now.",
- IllegalArgumentException.class,
- "Flink does not support 'range' write distribution mode now.",
- () -> {
- testWriteRow(null, DistributionMode.RANGE);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ testWriteRow(null, DistributionMode.RANGE);
+ })
+ .as("Does not support range distribution-mode now.")
Review Comment:
Addressed
--
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 v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1376216574
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkSourceConfig.java:
##########
@@ -31,14 +31,12 @@ public class TestFlinkSourceConfig extends TestFlinkTableSource {
@Test
public void testFlinkSessionConfig() {
getTableEnv().getConfig().set(FlinkReadOptions.STREAMING_OPTION, true);
- AssertHelpers.assertThrows(
- "Should throw exception because of cannot set snapshot-id option for streaming reader",
- IllegalArgumentException.class,
- "Cannot set as-of-timestamp option for streaming reader",
- () -> {
- sql("SELECT * FROM %s /*+ OPTIONS('as-of-timestamp'='1')*/", TABLE);
- return null;
- });
+ Assertions.assertThatThrownBy(
+ () -> {
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] Flink v1.15 : Remove usage of AssertHelpers [iceberg]
Posted by "coded9 (via GitHub)" <gi...@apache.org>.
coded9 commented on code in PR #8945:
URL: https://github.com/apache/iceberg/pull/8945#discussion_r1377358822
##########
flink/v1.15/flink/src/test/java/org/apache/iceberg/flink/source/TestStreamScanSql.java:
##########
@@ -306,15 +304,13 @@ public void testConsumeFromStartTag() throws Exception {
assertRows(ImmutableList.of(row7), iterator);
}
result.getJobClient().ifPresent(JobClient::cancel);
-
- AssertHelpers.assertThrows(
- "START_SNAPSHOT_ID and START_TAG cannot both be set.",
- IllegalArgumentException.class,
- "START_SNAPSHOT_ID and START_TAG cannot both be set.",
- () ->
- exec(
- "SELECT * FROM %s /*+ OPTIONS('streaming'='true', 'monitor-interval'='1s', 'start-tag'='%s', "
- + "'start-snapshot-id'='%d' )*/",
- TABLE, tagName, startSnapshotId));
+ Assertions.assertThatThrownBy(
+ () ->
+ exec(
+ "SELECT * FROM %s /*+ OPTIONS('streaming'='true', 'monitor-interval'='1s', 'start-tag'='%s', "
+ + "'start-snapshot-id'='%d' )*/",
+ TABLE, tagName, startSnapshotId))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("START_SNAPSHOT_ID and START_TAG cannot both be set.");
Review Comment:
Changed to use `.hasMessage()`
--
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