Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Core: Switch usage to DataFileSet / DeleteFileSet #11158

Merged
merged 4 commits into from
Oct 14, 2024
Merged

Core: Switch usage to DataFileSet / DeleteFileSet #11158

merged 4 commits into from
Oct 14, 2024

Conversation

nastra
Copy link
Contributor

@nastra nastra commented Sep 18, 2024 •
edited
Loading 

Usage of DataFileSet / DeleteFileSet
====================================

Benchmark                                       (numFiles)  Mode  Cnt  Score   Error  Units
ReplaceDeleteFilesBenchmark.replaceDeleteFiles       50000    ss    5  0.892 ± 1.837   s/op
ReplaceDeleteFilesBenchmark.replaceDeleteFiles      100000    ss    5  1.544 ± 0.139   s/op
ReplaceDeleteFilesBenchmark.replaceDeleteFiles      500000    ss    5  2.487 ± 0.542   s/op
ReplaceDeleteFilesBenchmark.replaceDeleteFiles     1000000    ss    5  2.534 ± 1.214   s/op
ReplaceDeleteFilesBenchmark.replaceDeleteFiles     2500000    ss    5  6.490 ± 2.507   s/op


main
=====
Benchmark                                       (numFiles)  Mode  Cnt  Score   Error  Units
ReplaceDeleteFilesBenchmark.replaceDeleteFiles       50000    ss    5  0.897 ± 1.884   s/op
ReplaceDeleteFilesBenchmark.replaceDeleteFiles      100000    ss    5  1.518 ± 0.133   s/op
ReplaceDeleteFilesBenchmark.replaceDeleteFiles      500000    ss    5  2.622 ± 0.862   s/op
ReplaceDeleteFilesBenchmark.replaceDeleteFiles     1000000    ss    5  2.540 ± 0.922   s/op
ReplaceDeleteFilesBenchmark.replaceDeleteFiles     2500000    ss    5  6.608 ± 3.326   s/op

@github-actions github-actions bot added the spark label Sep 18, 2024
@nastra nastra force-pushed the content-file-comparator branch 3 times, most recently from 5c25dd3 to 10150bf Compare September 18, 2024 13:12
@nastra nastra marked this pull request as draft September 18, 2024 13:56
@nastra nastra force-pushed the content-file-comparator branch 2 times, most recently from 1e0b413 to 54cffb4 Compare September 19, 2024 06:42
@nastra nastra changed the title Core: Add ContentFile comparator Core: Add Data/Delete file comparators Sep 19, 2024
@nastra nastra force-pushed the content-file-comparator branch 3 times, most recently from 4dbf58c to 73638a6 Compare September 19, 2024 13:58
@nastra nastra force-pushed the content-file-comparator branch 5 times, most recently from 70dcd1a to 6fc4e35 Compare September 19, 2024 14:36
@nastra nastra force-pushed the content-file-comparator branch 4 times, most recently from 1e6dfd7 to 8002680 Compare September 26, 2024 13:33
@nastra nastra changed the title Core: Add Data/Delete file comparators Core: Switch all places to use DataFileSet / DeleteFileSet Sep 30, 2024
@nastra nastra closed this Sep 30, 2024
@nastra nastra reopened this Sep 30, 2024
@nastra nastra force-pushed the content-file-comparator branch 2 times, most recently from 95d12ef to 08ccda9 Compare October 1, 2024 06:17
@github-actions github-actions bot removed the API label Oct 1, 2024
@nastra nastra changed the title Core: Switch all places to use DataFileSet / DeleteFileSet Core: Switch usage to DataFileSet / DeleteFileSet Oct 1, 2024
@nastra nastra marked this pull request as ready for review October 1, 2024 06:52
@nastra nastra mentioned this pull request Oct 4, 2024
Merged
aokolnychyi
aokolnychyi reviewed Oct 9, 2024
View reviewed changes
core/src/main/java/org/apache/iceberg/FastAppend.java Outdated Show resolved Hide resolved
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java Outdated Show resolved Hide resolved
core/src/main/java/org/apache/iceberg/ManifestFilterManager.java Outdated Show resolved Hide resolved
core/src/main/java/org/apache/iceberg/MergingSnapshotProducer.java
private final CharSequenceSet newDataFilePaths = CharSequenceSet.empty();
private final CharSequenceSet newDeleteFilePaths = CharSequenceSet.empty();
private final DataFileSet newDataFiles = DataFileSet.create();
private final DeleteFileSet newDeleteFiles = DeleteFileSet.create();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need these extra collections? Can't we use sets in newDataFilesBySpec and newDeleteFilesBySpec?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm handling this already in 4d42f18. I just didn't want to introduce too many changes/refactorings as the PR is already quite large

amogh-jahagirdar
amogh-jahagirdar reviewed Oct 9, 2024
View reviewed changes
Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @nastra offline, overall feel like the change is good but I think it's worth running some benchmarks as a sanity check. There really shouldn't be much of a change afterwards but it's more so to make sure there's not some regression we're not expecting.

@nastra nastra force-pushed the content-file-comparator branch 2 times, most recently from 83c6052 to 0df0066 Compare October 9, 2024 12:41
@nastra
Copy link
Contributor Author

nastra commented Oct 9, 2024

Discussed with @nastra offline, overall feel like the change is good but I think it's worth running some benchmarks as a sanity check. There really shouldn't be much of a change afterwards but it's more so to make sure there's not some regression we're not expecting.

@amogh-jahagirdar I added benchmark results to the PR description

aokolnychyi
aokolnychyi approved these changes Oct 14, 2024
View reviewed changes
hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java
@@ -213,7 +213,7 @@ public void testDropTable() throws IOException {
table.newAppend().appendFile(file1).appendFile(file2).commit();

// delete file2
table.newDelete().deleteFile(file2.path()).commit();
table.newDelete().deleteFile(file2).commit();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change required? There should be no behavior change, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a required change, but I wanted to move existing places to the non-path-based delete path

nessie/src/test/java/org/apache/iceberg/nessie/TestNessieTable.java
@@ -407,7 +407,7 @@ public void testDropTable() throws IOException {
table.newAppend().appendFile(file1).appendFile(file2).commit();

// delete file2
table.newDelete().deleteFile(file2.path()).commit();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not a required change, but I wanted to move existing places to the non-path-based delete path

core/src/main/java/org/apache/iceberg/ManifestFilterManager.java
@@ -71,9 +70,9 @@ public String partition() {
private final PartitionSet deleteFilePartitions;
private final PartitionSet dropPartitions;
private final CharSequenceSet deletePaths = CharSequenceSet.empty();
private final Set<F> deleteFiles = newFileSet();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we co-locate deleteFiles and deleteFilePartitions as deleteFilePartitions essentially contains a set of partitions derived from deleteFiles.

@aokolnychyi
Copy link
Contributor

Actually, the comments are minor and I verified that changes in tests are cosmetic locally. I'll go ahead and merge this one to unblock the subsequent PR.

@aokolnychyi aokolnychyi merged commit 6a5ae1a into apache:main Oct 14, 2024
49 checks passed
@aokolnychyi
Copy link
Contributor

Thanks, @nastra! Thanks for reviewing, @amogh-jahagirdar!

@nastra nastra deleted the content-file-comparator branch October 14, 2024 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amogh-jahagirdar amogh-jahagirdar amogh-jahagirdar left review comments

@aokolnychyi aokolnychyi aokolnychyi approved these changes

Assignees
No one assigned
Labels
core hive NESSIE spark
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nastra @aokolnychyi @amogh-jahagirdar