Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .palantir/revapi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,21 @@ acceptedBreaks:
- code: "java.method.addedToInterface"
new: "method ThisT org.apache.iceberg.SnapshotUpdate<ThisT>::scanManifestsWith(java.util.concurrent.ExecutorService)"
justification: "Accept all changes prior to introducing API compatibility checks"
- code: "java.method.addedToInterface"
new: "method java.lang.Iterable<org.apache.iceberg.DataFile> org.apache.iceberg.Snapshot::addedFiles(org.apache.iceberg.io.FileIO)"
justification: "Allow adding a new method to the interface - old method is deprecated"
- code: "java.method.addedToInterface"
new: "method java.lang.Iterable<org.apache.iceberg.DataFile> org.apache.iceberg.Snapshot::deletedFiles(org.apache.iceberg.io.FileIO)"
justification: "Allow adding a new method to the interface - old method is deprecated"
- code: "java.method.addedToInterface"
new: "method java.util.List<org.apache.iceberg.ManifestFile> org.apache.iceberg.Snapshot::allManifests(org.apache.iceberg.io.FileIO)"
justification: "Allow adding a new method to the interface - old method is deprecated"
- code: "java.method.addedToInterface"
new: "method java.util.List<org.apache.iceberg.ManifestFile> org.apache.iceberg.Snapshot::dataManifests(org.apache.iceberg.io.FileIO)"
justification: "Allow adding a new method to the interface - old method is deprecated"
- code: "java.method.addedToInterface"
new: "method java.util.List<org.apache.iceberg.ManifestFile> org.apache.iceberg.Snapshot::deleteManifests(org.apache.iceberg.io.FileIO)"
justification: "Allow adding a new method to the interface - old method is deprecated"
- code: "java.method.addedToInterface"
new: "method long org.apache.iceberg.actions.ExpireSnapshots.Result::deletedEqualityDeleteFilesCount()"
justification: "Interface is backward compatible, very unlikely anyone implements this Result bean interface"
Expand Down
62 changes: 62 additions & 0 deletions api/src/main/java/org/apache/iceberg/Snapshot.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.io.Serializable;
import java.util.List;
import java.util.Map;
import org.apache.iceberg.io.FileIO;

/**
* A snapshot of the data in a table at a point in time.
Expand Down Expand Up @@ -68,23 +69,58 @@ public interface Snapshot extends Serializable {
* Return all {@link ManifestFile} instances for either data or delete manifests in this snapshot.
*
* @return a list of ManifestFile
* @deprecated since 1.0.0 - Use {@link Snapshot#allManifests(FileIO)} instead.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of saying when it was deprecated, I think we should say when it will be removed. I'm thinking that will probably be 1.0.0, assuming that we do an 0.14.0 and then follow up with a 1.0.0 to remove deprecations, but open to other opinions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think that's a good idea. I believe Yufei did something similar (noting when it would be deprecated) when deprecating the old metadata write directory configurations.

I also think that this gives a little bit more of an argument for a 0.14.0. We can go through, determine what needs to be deprecated, cut a release, and then turn around and cut a 1.0.0 release that from then on has stronger API / ABI guarantees.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copied existing language and mentioned it's deprecated since 0.14.0 and will be deprecated in 1.0.0.

I found a deprecation notice saying something would be removed in 0.15.0, so I think some ambiguity about 0.14.0 vs 1.0.0 is fine for now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Also I’ll make a note of the 0.15.0 thing and we’ll get that cleared up. Found it basically git grep-Ing and it’s very unlikely we’ll have a 0.15.0.

*/
@Deprecated
List<ManifestFile> allManifests();

/**
* Return all {@link ManifestFile} instances for either data or delete manifests in this snapshot.
* <p>
* The FileIO instance is passed in for cases where the correct FileIO to use is not known when the
* TableMetadata (and thus BaseSnapshot implementations) is not known, such as parsing JSON for the
* RESTCatalog or for highly sessionzed applications like Trino or shared Spark servers.
Comment thread
kbendick marked this conversation as resolved.
Outdated
*
* @param io the {@link FileIO} instance to be used for reading Snapshot manifests, if not already cached
Comment thread
kbendick marked this conversation as resolved.
Outdated
* @return a list of ManifestFile
*/
List<ManifestFile> allManifests(FileIO io);

/**
* Return a {@link ManifestFile} for each data manifest in this snapshot.
*
* @return a list of ManifestFile
* @deprecated since 1.0.0 - Use {@link Snapshot#dataManifests(FileIO)} instead.
*/
@Deprecated
List<ManifestFile> dataManifests();

/**
* Return a {@link ManifestFile} for each data manifest in this snapshot.
*
* @param io the {@link FileIO} instance to be used for reading manifests, if not already cached
* @return a list of ManifestFile
*/
List<ManifestFile> dataManifests(FileIO io);

/**
* Return a {@link ManifestFile} for each delete manifest in this snapshot.
*
* @return a list of ManifestFile
* @deprecated since 1.0.0 - Use {@link Snapshot#deleteManifests(FileIO)} instead.
*/
@Deprecated
List<ManifestFile> deleteManifests();


/**
* Return a {@link ManifestFile} for each delete manifest in this snapshot.
*
* @param io the {@link FileIO} instance to be used for reading Snapshot manifests, if not already cached
* @return a list of ManifestFile
*/
List<ManifestFile> deleteManifests(FileIO io);

/**
* Return the name of the {@link DataOperations data operation} that produced this snapshot.
*
Expand All @@ -107,19 +143,45 @@ public interface Snapshot extends Serializable {
* record_count, and file_size_in_bytes. Other columns will be null.
*
* @return all files added to the table in this snapshot.
* @deprecated since 1.0.0 - Use {@link Snapshot#addedFiles(FileIO)} instead.
*/
@Deprecated
Iterable<DataFile> addedFiles();

/**
* Return all files added to the table in this snapshot.
* <p>
* The files returned include the following columns: file_path, file_format, partition,
* record_count, and file_size_in_bytes. Other columns will be null.
*
* @param io the {@link FileIO} instance to be used for reading files added in this snapshot, if not already cached
* @return all files added to the table in this snapshot.
*/
Iterable<DataFile> addedFiles(FileIO io);

/**
* Return all files deleted from the table in this snapshot.
* <p>
* The files returned include the following columns: file_path, file_format, partition,
* record_count, and file_size_in_bytes. Other columns will be null.
*
* @return all files deleted from the table in this snapshot.
* @deprecated since 1.0.0 - Use {@link Snapshot#deletedFiles(FileIO)} instead.
*/
@Deprecated
Iterable<DataFile> deletedFiles();

/**
* Return all files deleted from the table in this snapshot.
* <p>
* The files returned include the following columns: file_path, file_format, partition,
* record_count, and file_size_in_bytes. Other columns will be null.
*
* @param io the {@link FileIO} instance to be used for reading files deleted in this snapshot, if not already cached
* @return all files deleted from the table in this snapshot.
*/
Iterable<DataFile> deletedFiles(FileIO io);

/**
* Return the location of this snapshot's manifest list, or null if it is not separate.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ public void testParallelCommitMultiThreadSingleCommit() {

table.refresh();
Assert.assertEquals("Commits should all succeed sequentially", nThreads, table.history().size());
Assert.assertEquals("Should have all manifests", nThreads, table.currentSnapshot().allManifests().size());
Assert.assertEquals("Should have all manifests", nThreads,
table.currentSnapshot().allManifests(table.io()).size());
}

@Test
Expand Down Expand Up @@ -138,7 +139,7 @@ public void testParallelCommitMultiThreadMultiCommit() {

table.refresh();
Assert.assertEquals("Commits should all succeed sequentially", 20, table.history().size());
Assert.assertEquals("should have 20 manifests", 20, table.currentSnapshot().allManifests().size());
Assert.assertEquals("should have 20 manifests", 20, table.currentSnapshot().allManifests(table.io()).size());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ protected TableScan newRefinedScan(TableOperations ops, Table table, Schema sche

@Override
protected CloseableIterable<ManifestFile> manifests() {
return reachableManifests(Snapshot::dataManifests);
return reachableManifests(snapshot -> snapshot.dataManifests(tableOps().io()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ protected TableScan newRefinedScan(TableOperations ops, Table table, Schema sche

@Override
protected CloseableIterable<ManifestFile> manifests() {
return reachableManifests(Snapshot::deleteManifests);
return reachableManifests(snapshot -> snapshot.deleteManifests(tableOps().io()));
}
}
}
3 changes: 2 additions & 1 deletion core/src/main/java/org/apache/iceberg/AllEntriesTable.java
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ protected TableScan newRefinedScan(TableOperations ops, Table table, Schema sche

@Override
protected CloseableIterable<FileScanTask> doPlanFiles() {
CloseableIterable<ManifestFile> manifests = reachableManifests(Snapshot::allManifests);
CloseableIterable<ManifestFile> manifests =
reachableManifests(snapshot -> snapshot.allManifests(tableOps().io()));

String schemaString = SchemaParser.toJson(schema());
String specString = PartitionSpecParser.toJson(PartitionSpec.unpartitioned());
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/iceberg/AllFilesTable.java
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ protected TableScan newRefinedScan(TableOperations ops, Table table, Schema sche

@Override
protected CloseableIterable<ManifestFile> manifests() {
return reachableManifests(Snapshot::allManifests);
return reachableManifests(snapshot -> snapshot.allManifests(tableOps().io()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ protected CloseableIterable<FileScanTask> doPlanFiles() {
} else {
return StaticDataTask.of(
io.newInputFile(tableOps().current().metadataFileLocation()),
MANIFEST_FILE_SCHEMA, schema(), snap.allManifests(),
MANIFEST_FILE_SCHEMA, schema(), snap.allManifests(io),
manifest -> ManifestsTable.manifestFileToRow(specs.get(manifest.partitionSpecId()), manifest)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ private ManifestFile copyManifest(ManifestFile manifest) {

@Override
public List<ManifestFile> apply(TableMetadata base) {
List<ManifestFile> currentManifests = base.currentSnapshot().dataManifests();
List<ManifestFile> currentManifests = base.currentSnapshot().dataManifests(ops.io());
Set<ManifestFile> currentManifestSet = ImmutableSet.copyOf(currentManifests);

validateDeletedManifests(currentManifestSet);
Expand All @@ -183,7 +183,7 @@ public List<ManifestFile> apply(TableMetadata base) {
List<ManifestFile> apply = Lists.newArrayList();
Iterables.addAll(apply, newManifestsWithMetadata);
apply.addAll(keptManifests);
apply.addAll(base.currentSnapshot().deleteManifests());
apply.addAll(base.currentSnapshot().deleteManifests(ops.io()));

return apply;
}
Expand Down
Loading