amogh-jahagirdar commented on code in PR #17523:
URL: https://github.com/apache/iceberg/pull/17523#discussion_r3723665076
##########
core/src/main/java/org/apache/iceberg/BaseSnapshot.java:
##########
@@ -256,9 +256,14 @@ public Iterable<DeleteFile> removedDeleteFiles(FileIO
fileIO) {
return removedDeleteFiles;
}
+ @Override
+ public String snapshotFileLocation() {
Review Comment:
Maybe we should call this `rootLocation`? It feels weird to have to do
`snapshot.snapshotFileLocation()` because the method name sounds duplicative in
the context of already being within a snapshot. and I think `rootLocation` also
generalizes to v4+ and older format version as the manifest list can still be
considered a root of the metadata tree.
##########
api/src/main/java/org/apache/iceberg/SnapshotFile.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.encryption.EncryptionManager;
+
+/**
+ * The top-level file that a {@link Snapshot} points at. For v3 and earlier
this is a manifest list
+ * (see {@link ManifestListFile}); for v4+ it is a root manifest carrying a
mix of data-file entries
Review Comment:
Ah okay, for decryption we have a bunch of code that takes in
`ManifestListFile`
##########
api/src/main/java/org/apache/iceberg/SnapshotFile.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.encryption.EncryptionManager;
+
+/**
+ * The top-level file that a {@link Snapshot} points at. For v3 and earlier
this is a manifest list
+ * (see {@link ManifestListFile}); for v4+ it is a root manifest carrying a
mix of data-file entries
Review Comment:
I think same principle as my other naming comment, maybe we want to just
call this `RootFile`? It doesn't attach this interface too hard to the
"Snapshot" though of course for the forsseable future Snapshot will always have
a pointer to a single root. Not as opinionated on this one though
##########
api/src/main/java/org/apache/iceberg/SnapshotFile.java:
##########
@@ -0,0 +1,39 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg;
+
+import java.nio.ByteBuffer;
+import org.apache.iceberg.encryption.EncryptionManager;
+
+/**
+ * The top-level file that a {@link Snapshot} points at. For v3 and earlier
this is a manifest list
+ * (see {@link ManifestListFile}); for v4+ it is a root manifest carrying a
mix of data-file entries
Review Comment:
I guess I'm not following, why do we need this interface? Regardless of V4
or earlier we have a root file but we just need the location and all the other
things like encryption key etc should all just work as they do today.
##########
api/src/main/java/org/apache/iceberg/Snapshot.java:
##########
@@ -167,9 +167,22 @@ default Iterable<DeleteFile> removedDeleteFiles(FileIO io)
{
* Return the location of this snapshot's manifest list, or null if it is
not separate.
*
* @return the location of the manifest list for this Snapshot
+ * @deprecated since 1.13.0; use {@link #snapshotFileLocation()}, which
returns the manifest list
+ * for v3 and earlier and the root manifest for v4+.
*/
+ @Deprecated
String manifestListLocation();
+ /**
+ * Returns the location of this snapshot's top-level file — a manifest list
for v3 and earlier, or
+ * a root manifest for v4+.
+ *
+ * @return the location of the snapshot file for this Snapshot
+ */
+ default String snapshotFileLocation() {
Review Comment:
same as my other comment, I feel like we should just call it `rootLocation`?
It generalizes to both v3 and v4 imo and feels less awkawrd than
`snapshot.snapshotFileLocation`()?
--
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]
