You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "akshayakp97 (via GitHub)" <gi...@apache.org> on 2023/04/28 23:01:30 UTC
[GitHub] [iceberg] akshayakp97 opened a new pull request, #7468: Deprecate AssertHelpers - iceberg-orc, mr and api modules
akshayakp97 opened a new pull request, #7468:
URL: https://github.com/apache/iceberg/pull/7468
### Summary
- SubTask of https://github.com/apache/iceberg/issues/7094
- Remove AssertHelpers in `iceberg-orc`, `mr`, and `api` modules
--
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 #7468: Deprecate AssertHelpers - iceberg-orc, mr and api modules
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#issuecomment-1529535368
@akshayakp97 it seems there's an overlap with https://github.com/apache/iceberg/pull/7482, so you might want to sync up with @liuxiaocs7 on modules being 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
[GitHub] [iceberg] nastra commented on a diff in pull request #7468: API: Remove deprecated AssertHelpers usage
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1182186343
##########
api/src/test/java/org/apache/iceberg/TestSnapshotRef.java:
##########
@@ -67,52 +68,39 @@ public void testBranchWithOverride() {
@Test
public void testNoTypeFailure() {
- AssertHelpers.assertThrows(
- "Snapshot reference type must be specified",
- IllegalArgumentException.class,
- "Snapshot reference type must not be null",
- () -> SnapshotRef.builderFor(1L, null).build());
+ Assertions.assertThatThrownBy(() -> SnapshotRef.builderFor(1L, null).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Snapshot reference type must not be null");
Review Comment:
if possible we should prefer `.hasMessage(..)` where possible. For example, here the exact error msg ends up being `Snapshot reference type must not be null`, so there's no need to use `hasMessageContaining()`.
##########
api/src/test/java/org/apache/iceberg/io/TestCloseableIterable.java:
##########
@@ -91,10 +90,9 @@ public void testConcatWithEmptyIterables() {
// This will throw a NoSuchElementException
CloseableIterable<Integer> concat5 =
CloseableIterable.concat(Lists.newArrayList(empty, empty, empty));
- AssertHelpers.assertThrows(
- "should throw NoSuchElementException",
- NoSuchElementException.class,
- () -> Iterables.getLast(concat5));
+
+ Assertions.assertThatThrownBy(() -> Iterables.getLast(concat5))
+ .isInstanceOf(NoSuchElementException.class);
Review Comment:
would be good to check for the specific error msg here
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -731,46 +710,32 @@ public void testNotIn() {
@Test
public void testNotInExceptions() {
- AssertHelpers.assertThrows(
- "Throw exception if value is null",
- NullPointerException.class,
- "Cannot create expression literal from null",
- () -> notIn("x", (Literal) null));
-
- AssertHelpers.assertThrows(
- "Throw exception if value is null",
- NullPointerException.class,
- "Values cannot be null for NOT_IN predicate",
- () -> notIn("x", (Collection<?>) null));
-
- AssertHelpers.assertThrows(
- "Throw exception if calling literal() for IN predicate",
- IllegalArgumentException.class,
- "NOT_IN predicate cannot return a literal",
- () -> notIn("x", 5, 6).literal());
-
- AssertHelpers.assertThrows(
- "Throw exception if any value in the input is null",
- NullPointerException.class,
- "Cannot create expression literal from null",
- () -> notIn("x", 1, 2, null));
-
- AssertHelpers.assertThrows(
- "Throw exception if binding fails for any element in the set",
- ValidationException.class,
- "Invalid value for conversion to type int",
- () -> new Evaluator(STRUCT, notIn("x", 7, 8, 9.1)));
-
- AssertHelpers.assertThrows(
- "Throw exception if no input value",
- IllegalArgumentException.class,
- "Cannot create NOT_IN predicate without a value",
- () -> predicate(Expression.Operation.NOT_IN, "x"));
-
- AssertHelpers.assertThrows(
- "Implicit conversion NOT_IN to NOT_EQ and throw exception if binding fails",
- ValidationException.class,
- "Invalid value for conversion to type int",
- () -> new Evaluator(STRUCT, predicate(Expression.Operation.NOT_IN, "x", 5.1)));
+ Assertions.assertThatThrownBy(() -> notIn("x", (Literal) null))
+ .isInstanceOf(NullPointerException.class)
+ .hasMessageContaining("Cannot create expression literal from null");
+ Assertions.assertThatThrownBy(() -> notIn("x", (Collection<?>) null))
Review Comment:
newline missing
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -624,47 +622,28 @@ public void testIn() {
@Test
public void testInExceptions() {
- AssertHelpers.assertThrows(
- "Throw exception if value is null",
- NullPointerException.class,
- "Cannot create expression literal from null",
- () -> in("x", (Literal) null));
-
- AssertHelpers.assertThrows(
- "Throw exception if value is null",
- NullPointerException.class,
- "Values cannot be null for IN predicate",
- () -> in("x", (Collection<?>) null));
-
- AssertHelpers.assertThrows(
- "Throw exception if calling literal() for IN predicate",
- IllegalArgumentException.class,
- "IN predicate cannot return a literal",
- () -> in("x", 5, 6).literal());
-
- AssertHelpers.assertThrows(
- "Throw exception if any value in the input is null",
- NullPointerException.class,
- "Cannot create expression literal from null",
- () -> in("x", 1, 2, null));
-
- AssertHelpers.assertThrows(
- "Throw exception if binding fails for any element in the set",
- ValidationException.class,
- "Invalid value for conversion to type int",
- () -> new Evaluator(STRUCT, in("x", 7, 8, 9.1)));
-
- AssertHelpers.assertThrows(
- "Throw exception if no input value",
- IllegalArgumentException.class,
- "Cannot create IN predicate without a value",
- () -> predicate(Expression.Operation.IN, "x"));
-
- AssertHelpers.assertThrows(
- "Implicit conversion IN to EQ and throw exception if binding fails",
- ValidationException.class,
- "Invalid value for conversion to type int",
- () -> new Evaluator(STRUCT, predicate(Expression.Operation.IN, "x", 5.1)));
+ Assertions.assertThatThrownBy(() -> in("x", (Literal) null))
+ .isInstanceOf(NullPointerException.class)
+ .hasMessageContaining("Cannot create expression literal from null");
Review Comment:
better to add a newline after each statement and would also be good to double-check whether we can use `.hasMessage()` here everywhere
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionHelpers.java:
##########
@@ -174,20 +174,16 @@ public void testTransformExpressions() {
@Test
public void testNullName() {
- AssertHelpers.assertThrows(
- "Should catch null column names when creating expressions",
- NullPointerException.class,
- "Name cannot be null",
- () -> equal((String) null, 5));
+ Assertions.assertThatThrownBy(() -> equal((String) null, 5))
+ .isInstanceOf(NullPointerException.class)
+ .hasMessageContaining("Name cannot be null");
Review Comment:
`.hasMessage()`
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -450,20 +449,14 @@ public void testProjectListNested() {
required(17, "x", Types.IntegerType.get()),
required(18, "y", Types.IntegerType.get()))))))));
- AssertHelpers.assertThrows(
- "Cannot explicitly project List",
- IllegalArgumentException.class,
- () -> TypeUtil.project(schema, Sets.newHashSet(12)));
+ Assertions.assertThatThrownBy(() -> TypeUtil.project(schema, Sets.newHashSet(12)))
+ .isInstanceOf(IllegalArgumentException.class);
Review Comment:
these should all check for a specific error msg, as otherwise the called code could fail for a different reason that is being expected
##########
api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java:
##########
@@ -358,11 +358,9 @@ public void testUUIDHash() {
@Test
public void testVerifiedIllegalNumBuckets() {
- AssertHelpers.assertThrows(
- "Should fail if numBucket is less than or equal to zero",
- IllegalArgumentException.class,
- "Invalid number of buckets: 0 (must be > 0)",
- () -> Bucket.get(0));
+ Assertions.assertThatThrownBy(() -> Bucket.get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Invalid number of buckets: 0 (must be > 0)");
Review Comment:
`.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
[GitHub] [iceberg] jackye1995 commented on a diff in pull request #7468: Deprecate AssertHelpers - iceberg-orc, mr and api modules
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1181818789
##########
api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java:
##########
@@ -158,25 +119,55 @@ public void testMultipleDatePartitionsWithDifferentSourceColumns() {
@Test
public void testMultipleIdentityPartitions() {
PartitionSpec.builderFor(SCHEMA).year("d").identity("id").identity("d").identity("s").build();
- AssertHelpers.assertThrows(
- "Should not allow identity(id) and identity(id)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).identity("id").identity("id").build());
- AssertHelpers.assertThrows(
- "Should not allow identity(id) and identity(id, name)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).identity("id").identity("id", "test-id").build());
- AssertHelpers.assertThrows(
- "Should not allow identity(id) and identity(id, name)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () ->
- PartitionSpec.builderFor(SCHEMA)
- .identity("id", "test-id")
- .identity("d", "test-id")
- .build());
+ Assertions.assertThatThrownBy(
+ () -> PartitionSpec.builderFor(SCHEMA).identity("id").identity("id").build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot use partition name more than once");
+
+ Assertions.assertThatThrownBy(
+ () -> PartitionSpec.builderFor(SCHEMA).identity("id").identity("id", "test-id").build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot add redundant partition");
+
+ Assertions.assertThatThrownBy(
+ () ->
+ PartitionSpec.builderFor(SCHEMA)
+ .identity("id", "test-id")
+ .identity("d", "test-id")
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot use partition name more than once");
+ }
+
+ @Test
+ public void testMultipleDatePartitions() {
Review Comment:
can we avoid moving tests around? It changes the git diff order
--
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 #7468: API: Remove deprecated AssertHelpers usage
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1183283732
##########
api/src/test/java/org/apache/iceberg/io/TestCloseableIterable.java:
##########
@@ -91,10 +90,9 @@ public void testConcatWithEmptyIterables() {
// This will throw a NoSuchElementException
CloseableIterable<Integer> concat5 =
CloseableIterable.concat(Lists.newArrayList(empty, empty, empty));
- AssertHelpers.assertThrows(
- "should throw NoSuchElementException",
- NoSuchElementException.class,
- () -> Iterables.getLast(concat5));
+
+ Assertions.assertThatThrownBy(() -> Iterables.getLast(concat5))
+ .isInstanceOf(NoSuchElementException.class);
Review Comment:
ah ok, thanks for checking. Alternatively we could use `.hasMessage(null)` to indicate that there's no message I guess
--
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] akshayakp97 commented on a diff in pull request #7468: Deprecate AssertHelpers - iceberg-orc, mr and api modules
Posted by "akshayakp97 (via GitHub)" <gi...@apache.org>.
akshayakp97 commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1181819614
##########
api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java:
##########
@@ -158,25 +119,55 @@ public void testMultipleDatePartitionsWithDifferentSourceColumns() {
@Test
public void testMultipleIdentityPartitions() {
PartitionSpec.builderFor(SCHEMA).year("d").identity("id").identity("d").identity("s").build();
- AssertHelpers.assertThrows(
- "Should not allow identity(id) and identity(id)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).identity("id").identity("id").build());
- AssertHelpers.assertThrows(
- "Should not allow identity(id) and identity(id, name)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).identity("id").identity("id", "test-id").build());
- AssertHelpers.assertThrows(
- "Should not allow identity(id) and identity(id, name)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () ->
- PartitionSpec.builderFor(SCHEMA)
- .identity("id", "test-id")
- .identity("d", "test-id")
- .build());
+ Assertions.assertThatThrownBy(
+ () -> PartitionSpec.builderFor(SCHEMA).identity("id").identity("id").build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot use partition name more than once");
+
+ Assertions.assertThatThrownBy(
+ () -> PartitionSpec.builderFor(SCHEMA).identity("id").identity("id", "test-id").build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot add redundant partition");
+
+ Assertions.assertThatThrownBy(
+ () ->
+ PartitionSpec.builderFor(SCHEMA)
+ .identity("id", "test-id")
+ .identity("d", "test-id")
+ .build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Cannot use partition name more than once");
+ }
+
+ @Test
+ public void testMultipleDatePartitions() {
Review Comment:
yeah ill update this. sorry for the inconsistent diff
--
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] jackye1995 merged pull request #7468: API: Remove deprecated AssertHelpers usage
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #7468:
URL: https://github.com/apache/iceberg/pull/7468
--
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] jackye1995 commented on a diff in pull request #7468: Deprecate AssertHelpers - iceberg-orc, mr and api modules
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1181818154
##########
api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java:
##########
@@ -36,95 +37,55 @@ public class TestPartitionSpecValidation {
@Test
public void testMultipleTimestampPartitions() {
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and year(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").year("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow hour(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).hour("ts").hour("ts").build());
- }
+ Assertions.assertThatThrownBy(
Review Comment:
oh I see, nvm, the other ones are shown 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 a diff in pull request #7468: API: Remove deprecated AssertHelpers usage
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1183287013
##########
api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java:
##########
@@ -36,95 +37,86 @@ public class TestPartitionSpecValidation {
@Test
public void testMultipleTimestampPartitions() {
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and year(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").year("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow hour(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).hour("ts").hour("ts").build());
+ Assertions.assertThatThrownBy(
+ () -> PartitionSpec.builderFor(SCHEMA).year("ts").year("ts").build())
+ .hasMessageContaining("Cannot use partition name more than once")
+ .isInstanceOf(IllegalArgumentException.class);
Review Comment:
nit: in other places typically the `.hasMessage..` part comes after `.isInstanceOf()`, so we should probably make this consistent here as well
--
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] akshayakp97 commented on a diff in pull request #7468: API: Remove deprecated AssertHelpers usage
Posted by "akshayakp97 (via GitHub)" <gi...@apache.org>.
akshayakp97 commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1183032409
##########
api/src/test/java/org/apache/iceberg/io/TestCloseableIterable.java:
##########
@@ -91,10 +90,9 @@ public void testConcatWithEmptyIterables() {
// This will throw a NoSuchElementException
CloseableIterable<Integer> concat5 =
CloseableIterable.concat(Lists.newArrayList(empty, empty, empty));
- AssertHelpers.assertThrows(
- "should throw NoSuchElementException",
- NoSuchElementException.class,
- () -> Iterables.getLast(concat5));
+
+ Assertions.assertThatThrownBy(() -> Iterables.getLast(concat5))
+ .isInstanceOf(NoSuchElementException.class);
Review Comment:
thanks for the review. I don't see any error message in this case.
Refer - https://github.com/google/guava/blob/master/guava/src/com/google/common/collect/Iterables.java#L849
--
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 #7468: API: Remove deprecated AssertHelpers usage
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1182245984
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -450,20 +449,14 @@ public void testProjectListNested() {
required(17, "x", Types.IntegerType.get()),
required(18, "y", Types.IntegerType.get()))))))));
- AssertHelpers.assertThrows(
- "Cannot explicitly project List",
- IllegalArgumentException.class,
- () -> TypeUtil.project(schema, Sets.newHashSet(12)));
+ Assertions.assertThatThrownBy(() -> TypeUtil.project(schema, Sets.newHashSet(12)))
+ .isInstanceOf(IllegalArgumentException.class);
Review Comment:
these should all check for a specific error msg, as otherwise the called code could fail for a different reason than the one being 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
[GitHub] [iceberg] nastra commented on a diff in pull request #7468: API: Remove deprecated AssertHelpers usage
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1183287013
##########
api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java:
##########
@@ -36,95 +37,86 @@ public class TestPartitionSpecValidation {
@Test
public void testMultipleTimestampPartitions() {
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and year(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").year("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow hour(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).hour("ts").hour("ts").build());
+ Assertions.assertThatThrownBy(
+ () -> PartitionSpec.builderFor(SCHEMA).year("ts").year("ts").build())
+ .hasMessageContaining("Cannot use partition name more than once")
+ .isInstanceOf(IllegalArgumentException.class);
Review Comment:
nit: to be consistent with other places, the `.hasMessage..` part comes after `.isInstanceOf()`
--
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] jackye1995 commented on a diff in pull request #7468: Deprecate AssertHelpers - iceberg-orc, mr and api modules
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1181816607
##########
api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java:
##########
@@ -36,95 +37,55 @@ public class TestPartitionSpecValidation {
@Test
public void testMultipleTimestampPartitions() {
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and year(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").year("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow hour(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).hour("ts").hour("ts").build());
- }
+ Assertions.assertThatThrownBy(
Review Comment:
missing test cases?
--
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] akshayakp97 commented on pull request #7468: Deprecate AssertHelpers - iceberg-orc, mr and api modules
Posted by "akshayakp97 (via GitHub)" <gi...@apache.org>.
akshayakp97 commented on PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#issuecomment-1530008063
@nastra thanks for looking at my PR! I will update my PR to exclude the overlapping files from `orc` and `mr-hive` modules.
--
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] jackye1995 commented on a diff in pull request #7468: Deprecate AssertHelpers - iceberg-orc, mr and api modules
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1181817554
##########
api/src/test/java/org/apache/iceberg/TestPartitionSpecValidation.java:
##########
@@ -36,95 +37,55 @@ public class TestPartitionSpecValidation {
@Test
public void testMultipleTimestampPartitions() {
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and year(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").year("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow year(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).year("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and month(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").month("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow month(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).month("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and day(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").day("ts").build());
- AssertHelpers.assertThrows(
- "Should not allow day(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot add redundant partition",
- () -> PartitionSpec.builderFor(SCHEMA).day("ts").hour("ts").build());
-
- AssertHelpers.assertThrows(
- "Should not allow hour(ts) and hour(ts)",
- IllegalArgumentException.class,
- "Cannot use partition name more than once",
- () -> PartitionSpec.builderFor(SCHEMA).hour("ts").hour("ts").build());
- }
+ Assertions.assertThatThrownBy(
Review Comment:
I counted there are 16 tests originally, but only 10 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