Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
slfan1989 commented on PR #10189: URL: https://github.com/apache/ozone/pull/10189#issuecomment-4438077248 > @sreejasahithi thanks for the review, @slfan1989 thanks for the patch. @ashishkumar50 @sreejasahithi Thank you very much for your help with the 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
ashishkumar50 commented on PR #10189: URL: https://github.com/apache/ozone/pull/10189#issuecomment-4437161414 @sreejasahithi thanks for the review, @slfan1989 thanks for the patch. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
ashishkumar50 merged PR #10189: URL: https://github.com/apache/ozone/pull/10189 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
slfan1989 commented on code in PR #10189:
URL: https://github.com/apache/ozone/pull/10189#discussion_r3224904855
##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -188,6 +191,51 @@ void tablePathRewriteForStartAndEndVersionProvided()
throws Exception {
assertAllInternalPathsRewritten(csvPairs, targetPrefix);
}
+ @Test
+ void statsFileCopyPlanReturnsEmptySetForEmptyStats() {
+Set> copyPlan =
+RewriteTablePathOzoneUtils.statsFileCopyPlan(List.of(), List.of());
+
+assertTrue(copyPlan.isEmpty());
+ }
+
+ @Test
+ void statsFileCopyPlanRejectsMismatchedStatsCount() {
+IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class,
+() -> RewriteTablePathOzoneUtils.statsFileCopyPlan(
+List.of(statisticsFile("before-1.stats", 100)),
+List.of()));
+
+assertEquals("Before and after path rewrite, statistic files count should
be same",
+exception.getMessage());
+ }
+
+ @Test
+ void statsFileCopyPlanRejectsMismatchedStatsFileSize() {
+IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class,
+() -> RewriteTablePathOzoneUtils.statsFileCopyPlan(
+List.of(statisticsFile("before-1.stats", 100)),
+List.of(statisticsFile("after-1.stats", 200;
+
+assertEquals("Before and after path rewrite, statistic files size should
be same",
+exception.getMessage());
Review Comment:
Updated both assertions to use AssertJ hasMessageContaining.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
ashishkumar50 commented on code in PR #10189:
URL: https://github.com/apache/ozone/pull/10189#discussion_r3224309385
##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -188,6 +191,51 @@ void tablePathRewriteForStartAndEndVersionProvided()
throws Exception {
assertAllInternalPathsRewritten(csvPairs, targetPrefix);
}
+ @Test
+ void statsFileCopyPlanReturnsEmptySetForEmptyStats() {
+Set> copyPlan =
+RewriteTablePathOzoneUtils.statsFileCopyPlan(List.of(), List.of());
+
+assertTrue(copyPlan.isEmpty());
+ }
+
+ @Test
+ void statsFileCopyPlanRejectsMismatchedStatsCount() {
+IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class,
+() -> RewriteTablePathOzoneUtils.statsFileCopyPlan(
+List.of(statisticsFile("before-1.stats", 100)),
+List.of()));
+
+assertEquals("Before and after path rewrite, statistic files count should
be same",
+exception.getMessage());
Review Comment:
Use
`assertThat(exception).hasMessageContaining("Before and after path rewrite,
statistic files count should be same");`
##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -188,6 +191,51 @@ void tablePathRewriteForStartAndEndVersionProvided()
throws Exception {
assertAllInternalPathsRewritten(csvPairs, targetPrefix);
}
+ @Test
+ void statsFileCopyPlanReturnsEmptySetForEmptyStats() {
+Set> copyPlan =
+RewriteTablePathOzoneUtils.statsFileCopyPlan(List.of(), List.of());
+
+assertTrue(copyPlan.isEmpty());
+ }
+
+ @Test
+ void statsFileCopyPlanRejectsMismatchedStatsCount() {
+IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class,
+() -> RewriteTablePathOzoneUtils.statsFileCopyPlan(
+List.of(statisticsFile("before-1.stats", 100)),
+List.of()));
+
+assertEquals("Before and after path rewrite, statistic files count should
be same",
+exception.getMessage());
+ }
+
+ @Test
+ void statsFileCopyPlanRejectsMismatchedStatsFileSize() {
+IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class,
+() -> RewriteTablePathOzoneUtils.statsFileCopyPlan(
+List.of(statisticsFile("before-1.stats", 100)),
+List.of(statisticsFile("after-1.stats", 200;
+
+assertEquals("Before and after path rewrite, statistic files size should
be same",
+exception.getMessage());
Review Comment:
Use
`assertThat(exception).hasMessageContaining("Before and after path rewrite,
statistic files size should be same");`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
sreejasahithi commented on PR #10189: URL: https://github.com/apache/ozone/pull/10189#issuecomment-4422332768 @slfan1989 could you please resolve the conflicts. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
slfan1989 commented on code in PR #10189:
URL: https://github.com/apache/ozone/pull/10189#discussion_r3192722109
##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -188,6 +191,51 @@ void tablePathRewriteForStartAndEndVersionProvided()
throws Exception {
assertAllInternalPathsRewritten(csvPairs, targetPrefix);
}
+ @Test
+ void statsFileCopyPlanReturnsEmptySetForEmptyStats() {
+Set> copyPlan =
+RewriteTablePathOzoneUtils.statsFileCopyPlan(List.of(), List.of());
+
+assertTrue(copyPlan.isEmpty());
+ }
+
+ @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());
+ }
Review 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.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
sreejasahithi commented on code in PR #10189:
URL: https://github.com/apache/ozone/pull/10189#discussion_r3190747548
##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -188,6 +191,51 @@ void tablePathRewriteForStartAndEndVersionProvided()
throws Exception {
assertAllInternalPathsRewritten(csvPairs, targetPrefix);
}
+ @Test
+ void statsFileCopyPlanReturnsEmptySetForEmptyStats() {
+Set> copyPlan =
+RewriteTablePathOzoneUtils.statsFileCopyPlan(List.of(), List.of());
+
+assertTrue(copyPlan.isEmpty());
+ }
+
+ @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());
+ }
Review 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`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] HDDS-15174. Add tests for Ozone Iceberg statistics file copy plan. [ozone]
sreejasahithi commented on code in PR #10189:
URL: https://github.com/apache/ozone/pull/10189#discussion_r3190747548
##
hadoop-ozone/iceberg/src/test/java/org/apache/hadoop/ozone/iceberg/TestRewriteTablePathOzoneAction.java:
##
@@ -188,6 +191,51 @@ void tablePathRewriteForStartAndEndVersionProvided()
throws Exception {
assertAllInternalPathsRewritten(csvPairs, targetPrefix);
}
+ @Test
+ void statsFileCopyPlanReturnsEmptySetForEmptyStats() {
+Set> copyPlan =
+RewriteTablePathOzoneUtils.statsFileCopyPlan(List.of(), List.of());
+
+assertTrue(copyPlan.isEmpty());
+ }
+
+ @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());
+ }
Review Comment:
statistics files always have the `.stats` extension in real usage `.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`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
