You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "TQJADE (via GitHub)" <gi...@apache.org> on 2024/12/19 17:40:02 UTC
[PR] Remove number from assert description [iceberg]
TQJADE opened a new pull request, #11827:
URL: https://github.com/apache/iceberg/pull/11827
This follows the suggestions in: https://github.com/apache/iceberg/pull/11748/files/36d953f359e58d4f01f45a7783cf3ae01d52c59b#r1880821876
--
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] Remove number from assert description [iceberg]
Posted by "TQJADE (via GitHub)" <gi...@apache.org>.
TQJADE commented on code in PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#discussion_r1893187183
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -192,7 +192,7 @@ public void testUnpartitioned() throws Exception {
.option(SizeBasedFileRewriter.REWRITE_ALL, "true")
.execute();
List<DeleteFile> newDeleteFiles = deleteFiles(table);
- assertThat(newDeleteFiles).as("Expected 1 new delete file").hasSize(1);
+ assertThat(newDeleteFiles).as("Expected new delete file(s)").hasSize(1);
Review Comment:
Removed.
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -272,7 +272,7 @@ public void testRewriteFilter() throws Exception {
.execute();
List<DeleteFile> newDeleteFiles = except(deleteFiles(table), deleteFiles);
- assertThat(newDeleteFiles).as("Should have 4 delete files").hasSize(2);
+ assertThat(newDeleteFiles).as("Should have delete file(s)").hasSize(2);
Review Comment:
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] Remove number from assert description [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#issuecomment-2556439119
Thanks @TQJADE for your first Iceberg contribution , thanks @huaxingao for the guidance and review
--
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] Remove number from assert description [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#issuecomment-2556101594
@TQJADE Please run `./gradlew spotlessApply`to fix the format
--
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] Remove number from assert description [iceberg]
Posted by "TQJADE (via GitHub)" <gi...@apache.org>.
TQJADE commented on code in PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#discussion_r1893187447
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -453,7 +453,7 @@ public void testRewriteFilterRemoveDangling() throws Exception {
.execute();
List<DeleteFile> newDeleteFiles = except(deleteFiles(table), deleteFiles);
- assertThat(newDeleteFiles).as("Should have 2 new delete files").hasSize(0);
+ assertThat(newDeleteFiles).as("Should have no new delete files").hasSize(0);
Review Comment:
Done.
--
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] Remove number from assert description [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#discussion_r1892996768
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -453,7 +453,7 @@ public void testRewriteFilterRemoveDangling() throws Exception {
.execute();
List<DeleteFile> newDeleteFiles = except(deleteFiles(table), deleteFiles);
- assertThat(newDeleteFiles).as("Should have 2 new delete files").hasSize(0);
+ assertThat(newDeleteFiles).as("Should have no new delete files").hasSize(0);
Review Comment:
does isEmpty work for case where size = 0?
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -272,7 +272,7 @@ public void testRewriteFilter() throws Exception {
.execute();
List<DeleteFile> newDeleteFiles = except(deleteFiles(table), deleteFiles);
- assertThat(newDeleteFiles).as("Should have 4 delete files").hasSize(2);
+ assertThat(newDeleteFiles).as("Should have delete file(s)").hasSize(2);
Review Comment:
Same, can we remove language like 'should have'
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -192,7 +192,7 @@ public void testUnpartitioned() throws Exception {
.option(SizeBasedFileRewriter.REWRITE_ALL, "true")
.execute();
List<DeleteFile> newDeleteFiles = deleteFiles(table);
- assertThat(newDeleteFiles).as("Expected 1 new delete file").hasSize(1);
+ assertThat(newDeleteFiles).as("Expected new delete file(s)").hasSize(1);
Review Comment:
Can we remove language like 'expected'? I think assertThat already puts that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Remove number from assert description [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#discussion_r1893228523
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -380,7 +380,7 @@ public void testRemoveDanglingDeletes() throws Exception {
List<Object[]> actualRecords = records(table);
List<Object[]> actualDeletes = deleteRecords(table);
assertEquals("Rows must match", expectedRecords, actualRecords);
- assertThat(actualDeletes).as("Should be no new position deletes").hasSize(0);
+ assertThat(actualDeletes).as("No new position deletes").isEmpty();
Review Comment:
Do we need `no`? I wonder what the assert message is, maybe it will already say 'expected new position deletes to be empty'
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -1054,48 +1054,48 @@ private void checkResult(
List<DeleteFile> newDeletes,
int expectedGroups) {
assertThat(rewrittenDeletes.size())
- .as("Expected rewritten delete file count does not match")
+ .as("Rewritten delete file count does not match")
Review Comment:
How about get rid of 'does not match, match', again in my thought, the assert message will already say this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Remove number from assert description [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#issuecomment-2556101136
Can you run ```./gradlew spotlessApply```
--
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] Remove number from assert description [iceberg]
Posted by "TQJADE (via GitHub)" <gi...@apache.org>.
TQJADE commented on code in PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#discussion_r1893252492
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -380,7 +380,7 @@ public void testRemoveDanglingDeletes() throws Exception {
List<Object[]> actualRecords = records(table);
List<Object[]> actualDeletes = deleteRecords(table);
assertEquals("Rows must match", expectedRecords, actualRecords);
- assertThat(actualDeletes).as("Should be no new position deletes").hasSize(0);
+ assertThat(actualDeletes).as("No new position deletes").isEmpty();
Review Comment:
Remove. I tried to mock a failure, by changing isEmpty to isNotEmpty. Here is the messages with `No` started:
```
TestRewritePositionDeleteFilesAction > testRemoveDanglingDeletes() > catalogName = testhive, implementation = org.apache.iceberg.spark.SparkCatalog, config = {type=hive, default-namespace=default, cache-enabled=false}, fileFormat = PARQUET FAILED
java.lang.AssertionError: [No new position deletes]
Expecting actual not to be empty
at org.apache.iceberg.spark.actions.TestRewritePositionDeleteFilesAction.testRemoveDanglingDeletes(TestRewritePositionDeleteFilesAction.java:383)
```
--
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] Remove number from assert description [iceberg]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho merged PR #11827:
URL: https://github.com/apache/iceberg/pull/11827
--
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] Remove number from assert description [iceberg]
Posted by "TQJADE (via GitHub)" <gi...@apache.org>.
TQJADE commented on code in PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#discussion_r1892989980
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -310,7 +310,7 @@ public void testRewriteToSmallerTarget() throws Exception {
.option(SizeBasedFileRewriter.TARGET_FILE_SIZE_BYTES, String.valueOf(avgSize / 2))
.execute();
List<DeleteFile> newDeleteFiles = deleteFiles(table);
- assertThat(newDeleteFiles).as("Should have 8 new delete files").hasSize(8);
+ assertThat(newDeleteFiles).as("Should have new delete files").hasSize(8);
Review Comment:
Updated
--
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] Remove number from assert description [iceberg]
Posted by "TQJADE (via GitHub)" <gi...@apache.org>.
TQJADE commented on PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#issuecomment-2557293121
Thanks a lot for your reviewing @szehon-ho @huaxingao
--
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] Remove number from assert description [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#discussion_r1892939462
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -310,7 +310,7 @@ public void testRewriteToSmallerTarget() throws Exception {
.option(SizeBasedFileRewriter.TARGET_FILE_SIZE_BYTES, String.valueOf(avgSize / 2))
.execute();
List<DeleteFile> newDeleteFiles = deleteFiles(table);
- assertThat(newDeleteFiles).as("Should have 8 new delete files").hasSize(8);
+ assertThat(newDeleteFiles).as("Should have new delete files").hasSize(8);
Review Comment:
nit: Shall we unify the message and use `Expected new delete file(s)` as in line 195?
--
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] Remove number from assert description [iceberg]
Posted by "TQJADE (via GitHub)" <gi...@apache.org>.
TQJADE commented on code in PR #11827:
URL: https://github.com/apache/iceberg/pull/11827#discussion_r1893253207
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -1054,48 +1054,48 @@ private void checkResult(
List<DeleteFile> newDeletes,
int expectedGroups) {
assertThat(rewrittenDeletes.size())
- .as("Expected rewritten delete file count does not match")
+ .as("Rewritten delete file count does not match")
Review Comment:
Removed, then the failure logs will be like
```
java.lang.AssertionError: [XXX..: number of results should match]
Actual and expected should have same size but actual size is:
```
--
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