You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "amogh-jahagirdar (via GitHub)" <gi...@apache.org> on 2025/01/17 02:55:03 UTC
[PR] Spark: Fix Puffin suffix for DV files [iceberg]
amogh-jahagirdar opened a new pull request, #11986:
URL: https://github.com/apache/iceberg/pull/11986
Fixes #11968
Currently, when writing DVs in Spark, the incorrect file suffix is being used. This is because the file format input to the output file factory is not correct and uses the default conf delete file format (typically the same as the data file format).
Note, the actual DVs are being written, it just has the wrong suffix so users see "delete-foo.parquet" even though the file is a Puffin file with DVs.
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#issuecomment-2598427717
Thanks for reviewing @nastra @Fokko!
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919554870
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -723,7 +727,6 @@ public DeleteGranularity deleteGranularity() {
}
public boolean useDVs() {
- TableOperations ops = ((HasTableOperations) table).operations();
- return ops.current().formatVersion() >= 3;
+ return !(table instanceof BaseMetadataTable) && TableUtil.formatVersion(table) >= 3;
Review Comment:
I changed this because it is possible to pass in metadata tables to SparkWriteConf and then previously the useDVs check would fail, for cases like RewritePositionDeletes which passes in the PositionMetadataTable. Metadata tables by definition are read only and I think it doesn't make sense for the writeConf.useDVs() API to return true.
RewritePositionDeletes also uses metadataTable.deleteFileFormat() for determining the file format to write with but I didn't want to change anything additional on the deleteFileFormat() path since that's used in quite a bit more places...
I also refactored to use the new util which also handles the case where the table is a SerializableTable
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919554870
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -723,7 +727,6 @@ public DeleteGranularity deleteGranularity() {
}
public boolean useDVs() {
- TableOperations ops = ((HasTableOperations) table).operations();
- return ops.current().formatVersion() >= 3;
+ return !(table instanceof BaseMetadataTable) && TableUtil.formatVersion(table) >= 3;
Review Comment:
I changed this because it is possible to pass in metadata tables to SparkWriteConf and then previously the useDVs check would fail, for cases like RewritePositionDeletes which passes in the PositionMetadataTable. Metadata tables by definition are read only and I think it doesn't make sense for the writeConf.useDVs() API to return true.
RewritePositionDeletes also uses metadataTable.deleteFileFormat() for determining the file format to write with but I didn't want to change anything additional on the deleteFileFormat() path since that's used in quite a bit more places...
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919579513
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -723,7 +727,6 @@ public DeleteGranularity deleteGranularity() {
}
public boolean useDVs() {
Review Comment:
Changed my mind on second thought, see https://github.com/apache/iceberg/pull/11986#discussion_r1919571061 for my rationale
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919488304
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/source/SparkPositionDeltaWrite.java:
##########
@@ -798,14 +798,14 @@ private static class Context implements Serializable {
this.dataFileFormat = writeConf.dataFileFormat();
this.targetDataFileSize = writeConf.targetDataFileSize();
this.deleteSparkType = info.rowIdSchema().get();
- this.deleteFileFormat = writeConf.deleteFileFormat();
+ this.useDVs = writeConf.useDVs();
+ this.deleteFileFormat = useDVs ? FileFormat.PUFFIN : writeConf.deleteFileFormat();
Review Comment:
actually, we should probably just get this from the conf (embed the logic to determine if DVs are used there)
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919571061
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -721,9 +725,4 @@ public DeleteGranularity deleteGranularity() {
.defaultValue(DeleteGranularity.FILE)
.parse();
}
-
- public boolean useDVs() {
- TableOperations ops = ((HasTableOperations) table).operations();
- return ops.current().formatVersion() >= 3;
- }
Review Comment:
I ended up removing this since I realized it's not yet an officially released public API on SparkWriteConf. We can just update deleteFileFormat() to return PUFFIN and then use that in SparkPositionDeltaWrite, that simplifies the conf while solving the original issue of not surfacing the right format to the output file factory
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#issuecomment-2597330009
Working on a good place to add tests for this...we generally don't test against the actual file paths in Iceberg since they're not necessary for correctness but in this case it naturally causes confusion.
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919572654
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -214,6 +214,10 @@ private boolean fanoutWriterEnabled(boolean defaultValue) {
}
public FileFormat deleteFileFormat() {
+ if (!(table instanceof BaseMetadataTable) && TableUtil.formatVersion(table) >= 3) {
Review Comment:
See https://github.com/apache/iceberg/pull/11986#discussion_r1919554870 for why I changed 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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1924123333
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -214,6 +214,10 @@ private boolean fanoutWriterEnabled(boolean defaultValue) {
}
public FileFormat deleteFileFormat() {
+ if (!(table instanceof BaseMetadataTable) && TableUtil.formatVersion(table) >= 3) {
Review Comment:
We may not need `table instanceof BaseMetadataTable` once we add support for minor DV compaction. It looks good now.
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919564959
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -723,7 +727,6 @@ public DeleteGranularity deleteGranularity() {
}
public boolean useDVs() {
Review Comment:
It also feels like we should really just remove this method and have it embedded in deleteFileFormat().
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919564959
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -723,7 +727,6 @@ public DeleteGranularity deleteGranularity() {
}
public boolean useDVs() {
Review Comment:
It also feels like we should really just deprecate/remove this method and have it embedded in deleteFileFormat(). It's a bit more involved as a change (needs changes in `SparkPositionDeltaWrite`), so I'll leave it for now unless there's objections
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919680421
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -721,9 +725,4 @@ public DeleteGranularity deleteGranularity() {
.defaultValue(DeleteGranularity.FILE)
.parse();
}
-
- public boolean useDVs() {
- TableOperations ops = ((HasTableOperations) table).operations();
- return ops.current().formatVersion() >= 3;
- }
Review Comment:
SGTM, I had a similar concern here: https://github.com/apache/iceberg/pull/11588/files#r1916175700
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar merged PR #11986:
URL: https://github.com/apache/iceberg/pull/11986
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919571061
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -721,9 +725,4 @@ public DeleteGranularity deleteGranularity() {
.defaultValue(DeleteGranularity.FILE)
.parse();
}
-
- public boolean useDVs() {
- TableOperations ops = ((HasTableOperations) table).operations();
- return ops.current().formatVersion() >= 3;
- }
Review Comment:
I ended up removing this since I realized it's not an officially released public API on SparkWriteConf. We can just update deleteFileFormat() to return PUFFIN and then use that in SparkPositionDeltaWrite
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919564959
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -723,7 +727,6 @@ public DeleteGranularity deleteGranularity() {
}
public boolean useDVs() {
Review Comment:
It also feels like we should really just deprecate/remove this method and have it embedded in deleteFileFormat(). It's a bit more involved as a change, so I'll leave it for now unless there's objections
--
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] Spark: Fix Puffin suffix for DV files [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #11986:
URL: https://github.com/apache/iceberg/pull/11986#discussion_r1919564959
##########
spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/SparkWriteConf.java:
##########
@@ -723,7 +727,6 @@ public DeleteGranularity deleteGranularity() {
}
public boolean useDVs() {
Review Comment:
It also feels like we should really just deprecate/remove this method and have it embedded in deleteFileFormat().
--
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