Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24143 )
Change subject: IMPALA-14755: (part 2) Implement Iceberg deletion vector reading/writing ...................................................................... Patch Set 7: (13 comments) Thanks for applying the changes! Left a few comments, but there are still some parts that I need to take a deeper look. http://gerrit.cloudera.org:8080/#/c/24143/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24143/4//COMMIT_MSG@25 PS4, Line 25: equality-delete files > These can be allowed IIUC. As they can be added to the table at a later tim I think this has been resolved. Can we test it? http://gerrit.cloudera.org:8080/#/c/24143/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24143/7//COMMIT_MSG@24 PS7, Line 24: blocked nit: rejected? http://gerrit.cloudera.org:8080/#/c/24143/4/be/src/exec/iceberg-delete-builder.h File be/src/exec/iceberg-delete-builder.h: http://gerrit.cloudera.org:8080/#/c/24143/4/be/src/exec/iceberg-delete-builder.h@154 PS4, Line 154: deleted_rows_ > or we can reconsider enabling V3 deletes even if V2 pos deletes are there. The spec only lets you this if your new DV contains all delete information, as engines are free to ignore old position delete files if there is a DV for the data file. http://gerrit.cloudera.org:8080/#/c/24143/7/be/src/exec/iceberg-delete-builder.cc File be/src/exec/iceberg-delete-builder.cc: http://gerrit.cloudera.org:8080/#/c/24143/7/be/src/exec/iceberg-delete-builder.cc@205 PS7, Line 205: DeletionVectorBlobReader dv_loader dv_loader.fs_cache_ is recreated each time. Maybe we could create an fs_cache object outside of the loop, and give a reference to dv_loader. http://gerrit.cloudera.org:8080/#/c/24143/7/be/src/exec/iceberg-delete-builder.cc@192 PS7, Line 192: TIcebergDeletionVector tdel_vec; : tdel_vec.path = ice_del_vector->path()->str(); : tdel_vec.content_offset = ice_del_vector->content_offset(); : tdel_vec.content_size_in_bytes = ice_del_vector->content_size_in_bytes(); : if (tdel_vec.content_size_in_bytes <= 0) { : return Status(strings::Substitute( : "Invalid deletion vector for file '$0': content_size_in_bytes is $1.", : file_path_str, tdel_vec.content_size_in_bytes)); : } : VLOG_FILE << "Found DV for file " << file_path_str << " => " << tdel_vec.path; : RoaringBitmap64 bitmap; : { : SCOPED_TIMER(dv_load_timer_); : DeletionVectorBlobReader dv_loader; : RETURN_IF_ERROR(dv_loader.Load(reader_context_.get(), mem_tracker(), &obj_pool_, : tdel_vec.path, : tdel_vec.content_offset, : tdel_vec.content_size_in_bytes, &bitmap)); : } : deleted_rows_.emplace(std::piecewise_construct, : std::forward_as_tuple(ptr_copy, file_path_str.length()), : std::forward_as_tuple(std::move(bitmap))); nit: this could be moved to the above if stmt's body. Then L184 could be moved as well http://gerrit.cloudera.org:8080/#/c/24143/7/be/src/exec/iceberg-delete-sink-base.h File be/src/exec/iceberg-delete-sink-base.h: http://gerrit.cloudera.org:8080/#/c/24143/7/be/src/exec/iceberg-delete-sink-base.h@67 PS7, Line 67: std::map<THash128, TIcebergDeletionVector> referenced_deletion_vectors_ Can it just be a reference to the SinkConfig's map? http://gerrit.cloudera.org:8080/#/c/24143/7/be/src/exec/iceberg-delete-sink-base.cc File be/src/exec/iceberg-delete-sink-base.cc: http://gerrit.cloudera.org:8080/#/c/24143/7/be/src/exec/iceberg-delete-sink-base.cc@136 PS7, Line 136: output_partition->referenced_deletion_vectors Could it be a pointer/reference? http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java File fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java: http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/analysis/IcebergDeleteImpl.java@50 PS7, Line 50: == I think this should be >=, so we don't skip this test accidentally when we move to V4. http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java File fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java: http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/catalog/IcebergDeleteTable.java@90 PS7, Line 90: // PUFFIN format is used for deletion vectors, which are only supported in Iceberg I thought we only create IcebergDeleteTable for reading position delete files. We don't create it to read DVs. http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java File fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java: http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/catalog/iceberg/GroupedContentFiles.java@71 PS7, Line 71: // A data file can have at most one DV, but may also be covered by legacy : // position-delete files simultaneously (e.g. after a partial V3 migration : // by an external engine). Assert there is no second DV for this file. : Hash128 dataFileHash = IcebergUtil.getFilePathHash(dataFile); : Preconditions.checkState(!dataFileToDV.containsKey(dataFileHash), : "More than one deletion vector found for data file: %s", : dataFile.location()); : dataFileToDV.put(dataFileHash, : IcebergUtil.createTIcebergDeletionVector(delFile)); We could check that dataFileToDV.put() returns null. This way we avoid the extra call to containsKey(). http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java File fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java: http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/planner/IcebergScanNode.java@320 PS7, Line 320: dataFileToDV_ Would it make sense to store a FbIcebergDeletionVector object's ByteBuffer in the map? http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java File fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java: http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/planner/IcebergScanPlanner.java@740 PS7, Line 740: // A data file can have at most one DV, but may also be covered by legacy : // position-delete files simultaneously (e.g. after a partial V3 migration : // by an external engine). Assert there is no second DV for this file. : Hash128 dataFileHash = IcebergUtil.getFilePathHash(dataFile.location()); : Preconditions.checkState(!dataFileToDV_.containsKey(dataFileHash), : "More than one deletion vector found for data file: %s", : dataFile.location()); : dataFileToDV_.put(dataFileHash, We could check that dataFileToDV.put() returns null. This way we avoid the extra call to containsKey(). http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/24143/7/fe/src/main/java/org/apache/impala/service/IcebergCatalogOpExecutor.java@474 PS7, Line 474: DeleteFile deleteFile = createDeletionVector(feIcebergTable, dv); : rowDelta.addDeletes(deleteFile); We should add tests where we delete from a table that already has DVs written by Spark (and later Hive/Trino). E.g. we can use iceberg_v3_deletion_vectors. -- To view, visit http://gerrit.cloudera.org:8080/24143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5613c31a7aa46b94b7c70386c939c08cc68632cd Gerrit-Change-Number: 24143 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Rozsa <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Peter Rozsa <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 09 Apr 2026 14:48:40 +0000 Gerrit-HasComments: Yes
