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/03/17 17:38:55 UTC
[GitHub] [iceberg] liuxiaocs7 opened a new pull request, #7134: Issue 7094 data
liuxiaocs7 opened a new pull request, #7134:
URL: https://github.com/apache/iceberg/pull/7134
- SubTask of https://github.com/apache/iceberg/issues/7094
Remove `AssertHelpers` in `iceberg-data` module
--
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 pull request #7134: data: Remove AssertHelpers Usage
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 commented on PR #7134:
URL: https://github.com/apache/iceberg/pull/7134#issuecomment-1478412627
thanks for the work @liuxiaocs7 and thanks for the review @nastra !
--
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 #7134: data: Remove AssertHelpers Usage
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #7134:
URL: https://github.com/apache/iceberg/pull/7134#issuecomment-1477386194
@jackye1995 could you trigger CI and also take a look please?
--
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 #7134: data: Remove AssertHelpers Usage
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #7134:
URL: https://github.com/apache/iceberg/pull/7134#discussion_r1142537816
##########
data/src/test/java/org/apache/iceberg/io/TestPartitioningWriters.java:
##########
@@ -137,11 +137,10 @@ public void testClusteredDataWriterOutOfOrderPartitions() throws IOException {
writer.write(toRow(4, "bbb"), spec, partitionKey(spec, "bbb"));
writer.write(toRow(5, "ccc"), spec, partitionKey(spec, "ccc"));
- AssertHelpers.assertThrows(
- "Should fail to write out of order partitions",
- IllegalStateException.class,
- "Encountered records that belong to already closed files",
- () -> writer.write(toRow(6, "aaa"), spec, partitionKey(spec, "aaa")));
+ Assertions.assertThatThrownBy(
+ () -> writer.write(toRow(6, "aaa"), spec, partitionKey(spec, "aaa")))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessageContaining("Encountered records that belong to already closed files");
Review Comment:
Hi, @nastra thanks for your suggestion, I have added some more rules to make the check more strict, please take a look again for me in your free time, thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7134: data: Remove AssertHelpers Usage
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7134:
URL: https://github.com/apache/iceberg/pull/7134#discussion_r1141843630
##########
data/src/test/java/org/apache/iceberg/io/TestPartitioningWriters.java:
##########
@@ -137,11 +137,10 @@ public void testClusteredDataWriterOutOfOrderPartitions() throws IOException {
writer.write(toRow(4, "bbb"), spec, partitionKey(spec, "bbb"));
writer.write(toRow(5, "ccc"), spec, partitionKey(spec, "ccc"));
- AssertHelpers.assertThrows(
- "Should fail to write out of order partitions",
- IllegalStateException.class,
- "Encountered records that belong to already closed files",
- () -> writer.write(toRow(6, "aaa"), spec, partitionKey(spec, "aaa")));
+ Assertions.assertThatThrownBy(
+ () -> writer.write(toRow(6, "aaa"), spec, partitionKey(spec, "aaa")))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessageContaining("Encountered records that belong to already closed files");
Review Comment:
let's try and use `hasMessage` where possible and only fallback to `hasMessageContaining` for dynamic parts that change with every CI run (like time and such)
--
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 #7134: data: Remove AssertHelpers Usage
Posted by "liuxiaocs7 (via GitHub)" <gi...@apache.org>.
liuxiaocs7 commented on code in PR #7134:
URL: https://github.com/apache/iceberg/pull/7134#discussion_r1142416374
##########
data/src/test/java/org/apache/iceberg/io/TestPartitioningWriters.java:
##########
@@ -137,11 +137,10 @@ public void testClusteredDataWriterOutOfOrderPartitions() throws IOException {
writer.write(toRow(4, "bbb"), spec, partitionKey(spec, "bbb"));
writer.write(toRow(5, "ccc"), spec, partitionKey(spec, "ccc"));
- AssertHelpers.assertThrows(
- "Should fail to write out of order partitions",
- IllegalStateException.class,
- "Encountered records that belong to already closed files",
- () -> writer.write(toRow(6, "aaa"), spec, partitionKey(spec, "aaa")));
+ Assertions.assertThatThrownBy(
+ () -> writer.write(toRow(6, "aaa"), spec, partitionKey(spec, "aaa")))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessageContaining("Encountered records that belong to already closed files");
Review Comment:
Hi, @nastra, thanks for your advice, but here is the case with dynamic variables, i think we should continue to use `hasMessageContaining` because `write.write()` may raise throws the following exception.
```java
if (completedSpecIds.contains(spec.specId())) {
String errorCtx = String.format("spec %s", spec);
throw new IllegalStateException(NOT_CLUSTERED_ROWS_ERROR_MSG_TEMPLATE + errorCtx);
}
```
and `NOT_CLUSTERED_ROWS_ERROR_MSG_TEMPLATE` is:
```java
private static final String NOT_CLUSTERED_ROWS_ERROR_MSG_TEMPLATE =
"Incoming records violate the writer assumption that records are clustered by spec and "
+ "by partition within each spec. Either cluster the incoming records or switch to fanout writers.\n"
+ "Encountered records that belong to already closed files:\n";
```
Should we use the full constant string `NOT_CLUSTERED_ROWS_ERROR_MSG_TEMPLATE` to check? `
Encountered records that belong to already closed files` seems to be enough for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] jackye1995 merged pull request #7134: data: Remove AssertHelpers Usage
Posted by "jackye1995 (via GitHub)" <gi...@apache.org>.
jackye1995 merged PR #7134:
URL: https://github.com/apache/iceberg/pull/7134
--
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 #7134: data: Remove AssertHelpers Usage
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7134:
URL: https://github.com/apache/iceberg/pull/7134#discussion_r1142422717
##########
data/src/test/java/org/apache/iceberg/io/TestPartitioningWriters.java:
##########
@@ -137,11 +137,10 @@ public void testClusteredDataWriterOutOfOrderPartitions() throws IOException {
writer.write(toRow(4, "bbb"), spec, partitionKey(spec, "bbb"));
writer.write(toRow(5, "ccc"), spec, partitionKey(spec, "ccc"));
- AssertHelpers.assertThrows(
- "Should fail to write out of order partitions",
- IllegalStateException.class,
- "Encountered records that belong to already closed files",
- () -> writer.write(toRow(6, "aaa"), spec, partitionKey(spec, "aaa")));
+ Assertions.assertThatThrownBy(
+ () -> writer.write(toRow(6, "aaa"), spec, partitionKey(spec, "aaa")))
+ .isInstanceOf(IllegalStateException.class)
+ .hasMessageContaining("Encountered records that belong to already closed files");
Review Comment:
it's ok to use `hasMessageContaining` in such cases, I was just trying to point out that it would be generally good to re-visit the error msg check and make it as strict as possible (obviously where it makes sense)
--
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