You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "liuxiaocs7 (via GitHub)" <gi...@apache.org> on 2023/04/30 20:28:58 UTC
[GitHub] [iceberg] liuxiaocs7 opened a new pull request, #7483: Spark: Remove deprecated AssertHelpers
liuxiaocs7 opened a new pull request, #7483:
URL: https://github.com/apache/iceberg/pull/7483
- SubTask of https://github.com/apache/iceberg/issues/7094
Remove AssertHelpers in `iceberg-spark` module except `spark sql`
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7483: Spark: Remove deprecated AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7483:
URL: https://github.com/apache/iceberg/pull/7483#discussion_r1182248687
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestDataSourceOptions.java:
##########
@@ -242,50 +242,49 @@ public void testIncrementalScanOptions() throws IOException {
List<Long> snapshotIds = SnapshotUtil.currentAncestorIds(table);
// start-snapshot-id and snapshot-id are both configured.
- AssertHelpers.assertThrows(
- "Check both start-snapshot-id and snapshot-id are configured",
- IllegalArgumentException.class,
- "Cannot set start-snapshot-id and end-snapshot-id for incremental scans",
- () -> {
- spark
- .read()
- .format("iceberg")
- .option("snapshot-id", snapshotIds.get(3).toString())
- .option("start-snapshot-id", snapshotIds.get(3).toString())
- .load(tableLocation)
- .explain();
- });
+ Assertions.assertThatThrownBy(
+ () ->
+ spark
+ .read()
+ .format("iceberg")
+ .option("snapshot-id", snapshotIds.get(3).toString())
+ .option("start-snapshot-id", snapshotIds.get(3).toString())
+ .load(tableLocation)
+ .explain())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage(
+ "Cannot set start-snapshot-id and end-snapshot-id for incremental scans when either snapshot-id or as-of-timestamp is set");
// end-snapshot-id and as-of-timestamp are both configured.
- AssertHelpers.assertThrows(
- "Check both start-snapshot-id and snapshot-id are configured",
- IllegalArgumentException.class,
- "Cannot set start-snapshot-id and end-snapshot-id for incremental scans",
- () -> {
- spark
- .read()
- .format("iceberg")
- .option(
- SparkReadOptions.AS_OF_TIMESTAMP,
- Long.toString(table.snapshot(snapshotIds.get(3)).timestampMillis()))
- .option("end-snapshot-id", snapshotIds.get(2).toString())
- .load(tableLocation)
- .explain();
- });
+ Assertions.assertThatThrownBy(
+ () -> {
Review Comment:
here and below we can also remove the { } wrapping
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7483: Spark: Remove deprecated AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7483:
URL: https://github.com/apache/iceberg/pull/7483#discussion_r1181496355
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestDataSourceOptions.java:
##########
@@ -242,50 +242,50 @@ public void testIncrementalScanOptions() throws IOException {
List<Long> snapshotIds = SnapshotUtil.currentAncestorIds(table);
// start-snapshot-id and snapshot-id are both configured.
- AssertHelpers.assertThrows(
- "Check both start-snapshot-id and snapshot-id are configured",
- IllegalArgumentException.class,
- "Cannot set start-snapshot-id and end-snapshot-id for incremental scans",
- () -> {
- spark
- .read()
- .format("iceberg")
- .option("snapshot-id", snapshotIds.get(3).toString())
- .option("start-snapshot-id", snapshotIds.get(3).toString())
- .load(tableLocation)
- .explain();
- });
+ Assertions.assertThatThrownBy(
+ () -> {
Review Comment:
would be good to remove the { } and below around the actual call
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -76,18 +76,17 @@ public void testMergeSchemaFailsWithoutWriterOption() throws Exception {
// this has a different error message than the case without accept-any-schema because it uses
// Iceberg checks
- AssertHelpers.assertThrows(
- "Should fail when merge-schema is not enabled on the writer",
- IllegalArgumentException.class,
- "Field new_col not found in source schema",
- () -> {
- try {
- threeColDF.writeTo(tableName).append();
- } catch (NoSuchTableException e) {
- // needed because append has checked exceptions
- throw new RuntimeException(e);
- }
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ try {
Review Comment:
do we need this try-catch?
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -111,18 +110,18 @@ public void testMergeSchemaWithoutAcceptAnySchema() throws Exception {
"{ \"id\": 3, \"data\": \"c\", \"new_col\": 12.06 }",
"{ \"id\": 4, \"data\": \"d\", \"new_col\": 14.41 }");
- AssertHelpers.assertThrows(
- "Should fail when accept-any-schema is not enabled on the table",
- AnalysisException.class,
- "too many data columns",
- () -> {
- try {
- threeColDF.writeTo(tableName).option("merge-schema", "true").append();
- } catch (NoSuchTableException e) {
- // needed because append has checked exceptions
- throw new RuntimeException(e);
- }
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ try {
Review Comment:
do we need this try-catch block?
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestRequiredDistributionAndOrdering.java:
##########
@@ -162,20 +162,22 @@ public void testDisabledDistributionAndOrdering() {
Dataset<Row> inputDF = ds.coalesce(1).sortWithinPartitions("c1");
// should fail if ordering is disabled
- AssertHelpers.assertThrowsCause(
- "Should reject writes without ordering",
- IllegalStateException.class,
- "Incoming records violate the writer assumption",
- () -> {
- try {
- inputDF
- .writeTo(tableName)
- .option(SparkWriteOptions.USE_TABLE_DISTRIBUTION_AND_ORDERING, "false")
- .append();
- } catch (NoSuchTableException e) {
- throw new RuntimeException(e);
- }
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ try {
Review Comment:
do we need the try-catch block 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
[GitHub] [iceberg] liuxiaocs7 commented on a diff in pull request #7483: Spark: Remove deprecated AssertHelpers
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #7483:
URL: https://github.com/apache/iceberg/pull/7483#discussion_r1182259253
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestDataSourceOptions.java:
##########
@@ -242,50 +242,49 @@ public void testIncrementalScanOptions() throws IOException {
List<Long> snapshotIds = SnapshotUtil.currentAncestorIds(table);
// start-snapshot-id and snapshot-id are both configured.
- AssertHelpers.assertThrows(
- "Check both start-snapshot-id and snapshot-id are configured",
- IllegalArgumentException.class,
- "Cannot set start-snapshot-id and end-snapshot-id for incremental scans",
- () -> {
- spark
- .read()
- .format("iceberg")
- .option("snapshot-id", snapshotIds.get(3).toString())
- .option("start-snapshot-id", snapshotIds.get(3).toString())
- .load(tableLocation)
- .explain();
- });
+ Assertions.assertThatThrownBy(
+ () ->
+ spark
+ .read()
+ .format("iceberg")
+ .option("snapshot-id", snapshotIds.get(3).toString())
+ .option("start-snapshot-id", snapshotIds.get(3).toString())
+ .load(tableLocation)
+ .explain())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage(
+ "Cannot set start-snapshot-id and end-snapshot-id for incremental scans when either snapshot-id or as-of-timestamp is set");
// end-snapshot-id and as-of-timestamp are both configured.
- AssertHelpers.assertThrows(
- "Check both start-snapshot-id and snapshot-id are configured",
- IllegalArgumentException.class,
- "Cannot set start-snapshot-id and end-snapshot-id for incremental scans",
- () -> {
- spark
- .read()
- .format("iceberg")
- .option(
- SparkReadOptions.AS_OF_TIMESTAMP,
- Long.toString(table.snapshot(snapshotIds.get(3)).timestampMillis()))
- .option("end-snapshot-id", snapshotIds.get(2).toString())
- .load(tableLocation)
- .explain();
- });
+ Assertions.assertThatThrownBy(
+ () -> {
Review Comment:
thanks, fixed
--
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
[GitHub] [iceberg] liuxiaocs7 commented on a diff in pull request #7483: Spark: Remove deprecated AssertHelpers
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #7483:
URL: https://github.com/apache/iceberg/pull/7483#discussion_r1182132482
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -76,18 +76,17 @@ public void testMergeSchemaFailsWithoutWriterOption() throws Exception {
// this has a different error message than the case without accept-any-schema because it uses
// Iceberg checks
- AssertHelpers.assertThrows(
- "Should fail when merge-schema is not enabled on the writer",
- IllegalArgumentException.class,
- "Field new_col not found in source schema",
- () -> {
- try {
- threeColDF.writeTo(tableName).append();
- } catch (NoSuchTableException e) {
- // needed because append has checked exceptions
- throw new RuntimeException(e);
- }
- });
+ Assertions.assertThatThrownBy(
+ () -> {
+ try {
Review Comment:
Thanks comments, it can be remove, same as below
--
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
[GitHub] [iceberg] nastra commented on pull request #7483: Spark: Remove deprecated AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7483:
URL: https://github.com/apache/iceberg/pull/7483#issuecomment-1531411077
thanks @liuxiaocs7 for improving 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
[GitHub] [iceberg] nastra merged pull request #7483: Spark: Remove deprecated AssertHelpers
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7483:
URL: https://github.com/apache/iceberg/pull/7483
--
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