wombatu-kun commented on code in PR #20497:
URL: https://github.com/apache/hudi/pull/20497#discussion_r4450260264
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/TimelineUtils.java:
##########
@@ -420,6 +421,11 @@ public static void
validateTimestampAsOf(HoodieTableMetaClient metaClient, Strin
}
}
+ // A savepoint retains every file of its snapshot, so the cleaner cannot
have cleaned up a savepointed instant.
Review Comment:
`CleanPlanner` still marks a fully dropped partition for deletion without
checking savepoints, and `CleanActionExecutor` then deletes its directory
recursively, so after `ALTER TABLE DROP PARTITION` plus a clean, `TIMESTAMP AS
OF` a savepoint taken before the drop can silently omit that partition's rows
instead of being rejected. Can `CleanPlanner` skip `toDeletePartition` while a
savepoint lists files in that partition, here or in a prior PR, with a test
that reads the savepoint after the drop and the clean?
##########
hudi-common/src/main/java/org/apache/hudi/common/table/timeline/TimelineUtils.java:
##########
@@ -420,6 +421,11 @@ public static void
validateTimestampAsOf(HoodieTableMetaClient metaClient, Strin
}
}
+ // A savepoint retains every file of its snapshot, so the cleaner cannot
have cleaned up a savepointed instant.
+ if
(metaClient.getActiveTimeline().getSavePointTimeline().filterCompletedInstants().containsInstant(timestampAsOf))
{
Review Comment:
A savepoint taken while a clustering plan is pending pins the input file
group but not the clustered one, and the reader treats the input as replaced
because the plan was requested before the savepoint, so once a clean removes
the clustered slice, this return lets time travel silently drop those rows.
Should this fall through to the retention check when a replacecommit requested
at or before the savepoint completed after it?
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/dml/others/TestTimeTravelTable.scala:
##########
@@ -461,4 +461,59 @@ class TestTimeTravelTable extends HoodieSparkSqlTestBase {
subqueryError)
}
}
+
+ test("Test time travel to a savepointed commit after clean") {
Review Comment:
This test only covers COW, while other time travel tests here run on
`Seq("cow", "mor")` and a savepoint read on MOR has to merge a retained base
file with its logs. Can you add a MOR variant that compacts before the clean?
--
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]
