amogh-jahagirdar commented on code in PR #17497: URL: https://github.com/apache/iceberg/pull/17497#discussion_r3707862698
########## api/src/main/java/org/apache/iceberg/util/DeleteFileWrapper.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.util; + +import java.util.Objects; +import org.apache.iceberg.DeleteFile; + +/** + * Wrapper class to adapt DeleteFile for use in maps and sets. + * + * <p>Delete files are identified by location and content range rather than location alone, as a + * single Puffin file can hold a deletion vector for each of several data files. + */ +public class DeleteFileWrapper implements WrapperSet.Wrapper<DeleteFile> { Review Comment: I looked at other approaches (like how difficult it would be to make DeleteFileSet an indexed structure since it does store in insertion order, but removals get complicated etc). So I came to the conclusion that fundamentally we do need to expose DeleteFile equality as is done here via the wrapper. ########## core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java: ########## Review Comment: I was looking at this and in hindsight I think it would've been better to pass in Iterables for both of these. In the end this implementation takes the responsibility of tracking the positions, it's not like we need to rely on the caller to pass in an explicit List. ########## api/src/main/java/org/apache/iceberg/util/DeleteFileWrapper.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.util; + +import java.util.Objects; +import org.apache.iceberg.DeleteFile; + +/** + * Wrapper class to adapt DeleteFile for use in maps and sets. + * + * <p>Delete files are identified by location and content range rather than location alone, as a + * single Puffin file can hold a deletion vector for each of several data files. Review Comment: "single Puffin file can hold multiple DVs"? a DV is already known to be 1:1 for a data file so I don't think we need the "each of several data files" ########## core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java: ########## @@ -99,14 +100,14 @@ public static void serializeScanTasks( Map<Integer, PartitionSpec> specsById, JsonGenerator gen) throws IOException { - Map<String, Integer> deleteFilePathToIndex = Maps.newHashMap(); + Map<DeleteFileWrapper, Integer> deleteFileToIndex = Maps.newHashMap(); Review Comment: I would just add a 1 line comment maybe explaining DeleteFileWrapper is important here. ########## api/src/main/java/org/apache/iceberg/util/DeleteFileWrapper.java: ########## @@ -0,0 +1,77 @@ +/* + * 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.util; + +import java.util.Objects; +import org.apache.iceberg.DeleteFile; + +/** + * Wrapper class to adapt DeleteFile for use in maps and sets. + * + * <p>Delete files are identified by location and content range rather than location alone, as a + * single Puffin file can hold a deletion vector for each of several data files. + */ +public class DeleteFileWrapper implements WrapperSet.Wrapper<DeleteFile> { Review Comment: And I think any other kind of implementation would basically be a O(n) lookup when trying to find the index to serialize (the mapping from delete file to position in the protocol) into the response which feels needlessly suboptimal to just exposing this. ########## core/src/main/java/org/apache/iceberg/rest/TableScanResponseParser.java: ########## @@ -99,14 +100,14 @@ public static void serializeScanTasks( Map<Integer, PartitionSpec> specsById, JsonGenerator gen) throws IOException { - Map<String, Integer> deleteFilePathToIndex = Maps.newHashMap(); + Map<DeleteFileWrapper, Integer> deleteFileToIndex = Maps.newHashMap(); Review Comment: Normally, I think I'd have this map be defined to the interface `DeleteFile` (and the implementation just uses the DeleteFileWrapper when populating for correctness of the fix) but in this case I think it's actually better to define it to the concrete implementation as it's done here because it makes it clear that this really does need to be the wrapper for proper uniqueness comparison. It guarantees that if someone changes the below logic to populate some other type of DeleteFile that we're doing something wrong. -- 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]
