amogh-jahagirdar commented on code in PR #11478:
URL: https://github.com/apache/iceberg/pull/11478#discussion_r1837005354
##########
spark/v3.5/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestRewritePositionDeleteFilesProcedure.java:
##########
@@ -49,7 +49,7 @@ private void createTable(boolean partitioned) throws
Exception {
String partitionStmt = partitioned ? "PARTITIONED BY (id)" : "";
sql(
"CREATE TABLE %s (id bigint, data string) USING iceberg %s
TBLPROPERTIES"
- + "('format-version'='2', 'write.delete.mode'='merge-on-read')",
+ + "('format-version'='2', 'write.delete.mode'='merge-on-read',
'write.delete.granularity'='partitioned')",
Review Comment:
See below, the tests for the underlying action already test both partition +
file granularity so we have coverage of both cases. I don't know how much value
it would add to rewrite all the tests here which have expectations based on
partition granularity to have expectations based on file granularity or have
procedure level tests for both.
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/actions/TestRewritePositionDeleteFilesAction.java:
##########
@@ -741,7 +741,9 @@ private Map<String, String> tableProperties() {
TableProperties.FORMAT_VERSION,
"2",
TableProperties.DEFAULT_FILE_FORMAT,
- format.toString());
+ format.toString(),
+ TableProperties.DELETE_GRANULARITY,
+ DeleteGranularity.PARTITION.toString());
Review Comment:
This is just the default setup for a table in this test, we do test file
scoped deletes as well for this action in `testFileGranularity` in this test
class.
##########
spark/v3.4/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestDelete.java:
##########
@@ -154,7 +155,7 @@ public void testDeleteWithVectorizedReads() throws
NoSuchTableException {
}
@Test
- public void testCoalesceDelete() throws Exception {
+ public void testCoalesceDeleteWithPartitionGranularity() throws Exception {
Review Comment:
I updated this test to use partition scoped deletes since I think the
original intent of the test was to verify the output files when the shuffle
blocks are small and coalesced into a single task (with an unpartitioned table)
as part of AQE.
Without making this test specific to partition scoped deletes, we wouldn't
really be testing how AQE coalescing to a single task is affecting the number
of output files, which would be undifferentiated from the other tests where we
already cover file scoped deletes.
--
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]
