HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan.#10189
Conversation
sreejasahithi
left a comment
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @slfan1989 for adding tests for statistics file copy plan. left a minor comment below.
| @Test | ||
| void statsFileCopyPlanRejectsMismatchedStatsCount() { | ||
| IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, | ||
| () -> RewriteTablePathOzoneUtils.statsFileCopyPlan( | ||
| List.of(statisticsFile("before-1.puffin", 100)), | ||
| List.of())); | ||
|
|
||
| assertEquals("Before and after path rewrite, statistic files count should be same", | ||
| exception.getMessage()); | ||
| } |
There was a problem hiding this comment.
statistics files always have the .stats extension in real usage and .puffin is the extension used for Deletion Vector files (*-deletes.puffin). While the test works with any string since statsFileCopyPlan never validates the extension, using .puffin is misleading. Should be .stats
There was a problem hiding this comment.
Thanks for the review! Fixed. The test data now uses .stats for statistics files, which better matches the real file naming and avoids implying deletion vector files.
sreejasahithi
left a comment
sreejasahithi
left a comment
There was a problem hiding this comment.
Thanks @slfan1989
LGTM
|
@slfan1989 could you please resolve the conflicts. |
ashishkumar50
left a comment
ashishkumar50
left a comment
There was a problem hiding this comment.
@slfan1989 Thanks for adding the test, change LGTM except minor nits. Can you please resolve the conflict as well.
| List.of())); | ||
|
|
||
| assertEquals("Before and after path rewrite, statistic files count should be same", | ||
| exception.getMessage()); |
There was a problem hiding this comment.
Use
assertThat(exception).hasMessageContaining("Before and after path rewrite, statistic files count should be same");
| List.of(statisticsFile("after-1.stats", 200)))); | ||
|
|
||
| assertEquals("Before and after path rewrite, statistic files size should be same", | ||
| exception.getMessage()); |
There was a problem hiding this comment.
Use
assertThat(exception).hasMessageContaining("Before and after path rewrite, statistic files size should be same");
There was a problem hiding this comment.
Updated both assertions to use AssertJ hasMessageContaining.
|
@sreejasahithi thanks for the review, @slfan1989 thanks for the patch. |
@ashishkumar50 @sreejasahithi Thank you very much for your help with the review! |
5 participants
What changes were proposed in this pull request?
This PR adds unit test coverage for the Ozone Iceberg statistics file copy plan helper.
RewriteTablePathOzoneUtils.statsFileCopyPlanis used by the Ozone IcebergRewriteTablePathimplementation to build copy plan entries for table statistics files. The helper validates that the original and rewritten statistics file lists match in count and file size, and then creates mappings from the original statistics file paths to the rewritten statistics file paths.This PR adds tests for the following cases:
The change improves coverage for the Iceberg rewrite path utility without changing production behavior.
What is the link to the Apache JIRA
HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan.
How was this patch tested?