mbutrovich commented on code in PR #18464: URL: https://github.com/apache/iceberg/pull/18464#discussion_r4285781987
########## core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java: ########## @@ -0,0 +1,358 @@ +/* + * 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 static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.util.Map; +import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileMetadata; +import org.apache.iceberg.MetadataColumns; +import org.apache.iceberg.Metrics; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RewriteTablePathUtil; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.types.Conversions; +import org.apache.iceberg.types.Type; +import org.junit.jupiter.api.Test; + +public class TestContentFileUtil { + + private static final PartitionSpec SPEC = PartitionSpec.unpartitioned(); + private static final int PATH_ID = MetadataColumns.DELETE_FILE_PATH.fieldId(); + private static final Type PATH_TYPE = MetadataColumns.DELETE_FILE_PATH.type(); + private static final int POS_ID = MetadataColumns.DELETE_FILE_POS.fieldId(); + + @Test + public void referencedDataFileReturnsNullForEqualityDeletes() { + DeleteFile equalityDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofEqualityDeletes(1) + .withPath("/path/to/equality-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(equalityDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsExplicitReference() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)) + .isEqualTo("/path/to/data.parquet"); + } + + @Test + public void referencedDataFileReturnsPathFromEqualBounds() { + String dataFile = "/path/to/data.parquet"; + DeleteFile positionDelete = positionDeleteWithPathBounds(dataFile, dataFile); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).hasToString(dataFile); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsDiffer() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/path/to/data-1.parquet", "/path/to/data-2.parquet"); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsMissing() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } Review Comment: Do we need this one? Existing scan planning tests already exercise a v2 position delete with null bound maps. [`testPartitionedTableWithPartitionPosDeletes`](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/test/java/org/apache/iceberg/DeleteFileIndexTestBase.java#L349-L366) plans with [`FILE_A_DELETES`](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/test/java/org/apache/iceberg/TestBase.java#L89-L96), which has no metrics, and `DeleteFileIndex` calls [`referencedDataFileLocation`](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/main/java/org/apache/iceberg/DeleteFileIndex.java#L582-L591) on it to pick the partition index. ########## core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java: ########## @@ -0,0 +1,358 @@ +/* + * 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 static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.util.Map; +import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileMetadata; +import org.apache.iceberg.MetadataColumns; +import org.apache.iceberg.Metrics; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RewriteTablePathUtil; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.types.Conversions; +import org.apache.iceberg.types.Type; +import org.junit.jupiter.api.Test; + +public class TestContentFileUtil { + + private static final PartitionSpec SPEC = PartitionSpec.unpartitioned(); + private static final int PATH_ID = MetadataColumns.DELETE_FILE_PATH.fieldId(); + private static final Type PATH_TYPE = MetadataColumns.DELETE_FILE_PATH.type(); + private static final int POS_ID = MetadataColumns.DELETE_FILE_POS.fieldId(); + + @Test + public void referencedDataFileReturnsNullForEqualityDeletes() { + DeleteFile equalityDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofEqualityDeletes(1) + .withPath("/path/to/equality-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(equalityDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsExplicitReference() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)) + .isEqualTo("/path/to/data.parquet"); + } + + @Test + public void referencedDataFileReturnsPathFromEqualBounds() { + String dataFile = "/path/to/data.parquet"; + DeleteFile positionDelete = positionDeleteWithPathBounds(dataFile, dataFile); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).hasToString(dataFile); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsDiffer() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/path/to/data-1.parquet", "/path/to/data-2.parquet"); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsMissing() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenUpperBoundMissing() { + // lower bound carries the path, but the upper bound for the path column is absent + Map<Integer, ByteBuffer> lowerBounds = Maps.newHashMap(); + lowerBounds.put(PATH_ID, pathBound("/path/to/data.parquet")); + Map<Integer, ByteBuffer> upperBounds = Maps.newHashMap(); + upperBounds.put(POS_ID, posBound(10L)); + DeleteFile positionDelete = positionDeleteWithBounds(lowerBounds, upperBounds); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } Review Comment: Could we add the mirror case, with the lower `file_path` bound missing and the upper bound present? No test reaches the [`lowerPathBound == null`](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java#L74) check with an upper bound set. If that check were removed, [`lowerPathBound.equals(upperPathBound)`](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java#L84) would throw a `NullPointerException` and every test here would still pass. `replacePathBounds` has the same shape, so a matching case next to `replacePathBoundsClearsWhenUpperBoundMissing` would cover it there too. ```suggestion @Test public void referencedDataFileReturnsNullWhenUpperBoundMissing() { // lower bound carries the path, but the upper bound for the path column is absent Map<Integer, ByteBuffer> lowerBounds = Maps.newHashMap(); lowerBounds.put(PATH_ID, pathBound("/path/to/data.parquet")); Map<Integer, ByteBuffer> upperBounds = Maps.newHashMap(); upperBounds.put(POS_ID, posBound(10L)); DeleteFile positionDelete = positionDeleteWithBounds(lowerBounds, upperBounds); assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); } @Test public void referencedDataFileReturnsNullWhenLowerBoundMissing() { // upper bound carries the path, but the lower bound for the path column is absent Map<Integer, ByteBuffer> lowerBounds = Maps.newHashMap(); lowerBounds.put(POS_ID, posBound(0L)); Map<Integer, ByteBuffer> upperBounds = Maps.newHashMap(); upperBounds.put(PATH_ID, pathBound("/path/to/data.parquet")); DeleteFile positionDelete = positionDeleteWithBounds(lowerBounds, upperBounds); assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); } ``` ########## core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java: ########## @@ -0,0 +1,358 @@ +/* + * 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 static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.util.Map; +import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileMetadata; +import org.apache.iceberg.MetadataColumns; +import org.apache.iceberg.Metrics; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RewriteTablePathUtil; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.types.Conversions; +import org.apache.iceberg.types.Type; +import org.junit.jupiter.api.Test; + +public class TestContentFileUtil { + + private static final PartitionSpec SPEC = PartitionSpec.unpartitioned(); + private static final int PATH_ID = MetadataColumns.DELETE_FILE_PATH.fieldId(); + private static final Type PATH_TYPE = MetadataColumns.DELETE_FILE_PATH.type(); + private static final int POS_ID = MetadataColumns.DELETE_FILE_POS.fieldId(); + + @Test + public void referencedDataFileReturnsNullForEqualityDeletes() { + DeleteFile equalityDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofEqualityDeletes(1) + .withPath("/path/to/equality-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(equalityDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsExplicitReference() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)) + .isEqualTo("/path/to/data.parquet"); + } + + @Test + public void referencedDataFileReturnsPathFromEqualBounds() { + String dataFile = "/path/to/data.parquet"; + DeleteFile positionDelete = positionDeleteWithPathBounds(dataFile, dataFile); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).hasToString(dataFile); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsDiffer() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/path/to/data-1.parquet", "/path/to/data-2.parquet"); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsMissing() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenUpperBoundMissing() { + // lower bound carries the path, but the upper bound for the path column is absent + Map<Integer, ByteBuffer> lowerBounds = Maps.newHashMap(); + lowerBounds.put(PATH_ID, pathBound("/path/to/data.parquet")); + Map<Integer, ByteBuffer> upperBounds = Maps.newHashMap(); + upperBounds.put(POS_ID, posBound(10L)); + DeleteFile positionDelete = positionDeleteWithBounds(lowerBounds, upperBounds); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileLocation() { + DeleteFile withRef = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + DeleteFile withoutRef = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFileLocation(withRef)) + .isEqualTo("/path/to/data.parquet"); + assertThat(ContentFileUtil.referencedDataFileLocation(withoutRef)).isNull(); + } + + @Test + public void isFileScoped() { + DeleteFile fileScoped = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + DeleteFile notFileScoped = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.isFileScoped(fileScoped)).isTrue(); + assertThat(ContentFileUtil.isFileScoped(notFileScoped)).isFalse(); + } + + @Test + public void isDV() { + assertThat(ContentFileUtil.isDV(dv())).isTrue(); + + DeleteFile parquetDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + assertThat(ContentFileUtil.isDV(parquetDelete)).isFalse(); + } + + @Test + public void containsSingleDV() { + DeleteFile dv = dv(); + DeleteFile parquetDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(dv))).isTrue(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of())).isFalse(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(dv(), dv()))).isFalse(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(parquetDelete))).isFalse(); + } + + @Test + public void dvDesc() { + DeleteFile dv = dv(); + + assertThat(ContentFileUtil.dvDesc(dv)) + .contains("location=" + dv.location()) + .contains("offset=" + dv.contentOffset()) + .contains("length=" + dv.contentSizeInBytes()) + .contains("referencedDataFile=" + dv.referencedDataFile()); + } + + @Test + public void replacePathBoundsRejectsNonPositionDeletes() { + DeleteFile equalityDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofEqualityDeletes(1) + .withPath("/path/to/equality-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThatThrownBy(() -> ContentFileUtil.replacePathBounds(equalityDelete, "/old", "/new")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Only position delete files supported"); + } + + @Test + public void replacePathBoundsRewritesEqualBounds() { + String sourcePrefix = "/old/table"; + String targetPrefix = "/new/table"; + String dataFile = sourcePrefix + "/data/file.parquet"; + DeleteFile positionDelete = positionDeleteWithPathBounds(dataFile, dataFile); + + Metrics metrics = ContentFileUtil.replacePathBounds(positionDelete, sourcePrefix, targetPrefix); + + String expected = RewriteTablePathUtil.newPath(dataFile, sourcePrefix, targetPrefix); + assertThat(expected).isNotEqualTo(dataFile); Review Comment: Should the expected path be a literal? `replacePathBounds` calls `RewriteTablePathUtil.newPath` itself, so computing the expected value with it means a bug in `newPath` would pass here. With a literal, the `isNotEqualTo` check isn't needed, and the `RewriteTablePathUtil` import on line 31 becomes unused. ```suggestion String expected = "/new/table/data/file.parquet"; ``` ########## core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java: ########## @@ -0,0 +1,358 @@ +/* + * 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 static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.util.Map; +import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileMetadata; +import org.apache.iceberg.MetadataColumns; +import org.apache.iceberg.Metrics; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RewriteTablePathUtil; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.types.Conversions; +import org.apache.iceberg.types.Type; +import org.junit.jupiter.api.Test; + +public class TestContentFileUtil { + + private static final PartitionSpec SPEC = PartitionSpec.unpartitioned(); + private static final int PATH_ID = MetadataColumns.DELETE_FILE_PATH.fieldId(); + private static final Type PATH_TYPE = MetadataColumns.DELETE_FILE_PATH.type(); + private static final int POS_ID = MetadataColumns.DELETE_FILE_POS.fieldId(); + + @Test + public void referencedDataFileReturnsNullForEqualityDeletes() { + DeleteFile equalityDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofEqualityDeletes(1) + .withPath("/path/to/equality-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); Review Comment: This delete has no referenced data file and no `file_path` bounds, so `referencedDataFile` returns null here even without the [`EQUALITY_DELETES` early return](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/main/java/org/apache/iceberg/util/ContentFileUtil.java#L64-L66). Should it set a referenced data file, so the test fails if that branch goes away? ```suggestion DeleteFile equalityDelete = FileMetadata.deleteFileBuilder(SPEC) .ofEqualityDeletes(1) .withPath("/path/to/equality-delete.parquet") .withFileSizeInBytes(1024) .withRecordCount(5) .withReferencedDataFile("/path/to/data.parquet") .build(); ``` ########## core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java: ########## @@ -0,0 +1,358 @@ +/* + * 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 static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.util.Map; +import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileMetadata; +import org.apache.iceberg.MetadataColumns; +import org.apache.iceberg.Metrics; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RewriteTablePathUtil; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.types.Conversions; +import org.apache.iceberg.types.Type; +import org.junit.jupiter.api.Test; + +public class TestContentFileUtil { + + private static final PartitionSpec SPEC = PartitionSpec.unpartitioned(); + private static final int PATH_ID = MetadataColumns.DELETE_FILE_PATH.fieldId(); + private static final Type PATH_TYPE = MetadataColumns.DELETE_FILE_PATH.type(); + private static final int POS_ID = MetadataColumns.DELETE_FILE_POS.fieldId(); + + @Test + public void referencedDataFileReturnsNullForEqualityDeletes() { + DeleteFile equalityDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofEqualityDeletes(1) + .withPath("/path/to/equality-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(equalityDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsExplicitReference() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)) + .isEqualTo("/path/to/data.parquet"); + } + + @Test + public void referencedDataFileReturnsPathFromEqualBounds() { + String dataFile = "/path/to/data.parquet"; + DeleteFile positionDelete = positionDeleteWithPathBounds(dataFile, dataFile); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).hasToString(dataFile); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsDiffer() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/path/to/data-1.parquet", "/path/to/data-2.parquet"); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsMissing() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenUpperBoundMissing() { + // lower bound carries the path, but the upper bound for the path column is absent + Map<Integer, ByteBuffer> lowerBounds = Maps.newHashMap(); + lowerBounds.put(PATH_ID, pathBound("/path/to/data.parquet")); + Map<Integer, ByteBuffer> upperBounds = Maps.newHashMap(); + upperBounds.put(POS_ID, posBound(10L)); + DeleteFile positionDelete = positionDeleteWithBounds(lowerBounds, upperBounds); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileLocation() { + DeleteFile withRef = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + DeleteFile withoutRef = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFileLocation(withRef)) + .isEqualTo("/path/to/data.parquet"); + assertThat(ContentFileUtil.referencedDataFileLocation(withoutRef)).isNull(); + } + + @Test + public void isFileScoped() { + DeleteFile fileScoped = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + DeleteFile notFileScoped = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.isFileScoped(fileScoped)).isTrue(); + assertThat(ContentFileUtil.isFileScoped(notFileScoped)).isFalse(); + } + + @Test + public void isDV() { + assertThat(ContentFileUtil.isDV(dv())).isTrue(); + + DeleteFile parquetDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + assertThat(ContentFileUtil.isDV(parquetDelete)).isFalse(); + } + + @Test + public void containsSingleDV() { + DeleteFile dv = dv(); + DeleteFile parquetDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(dv))).isTrue(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of())).isFalse(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(dv(), dv()))).isFalse(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(parquetDelete))).isFalse(); + } + + @Test + public void dvDesc() { + DeleteFile dv = dv(); + + assertThat(ContentFileUtil.dvDesc(dv)) + .contains("location=" + dv.location()) + .contains("offset=" + dv.contentOffset()) + .contains("length=" + dv.contentSizeInBytes()) + .contains("referencedDataFile=" + dv.referencedDataFile()); + } + + @Test + public void replacePathBoundsRejectsNonPositionDeletes() { + DeleteFile equalityDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofEqualityDeletes(1) + .withPath("/path/to/equality-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThatThrownBy(() -> ContentFileUtil.replacePathBounds(equalityDelete, "/old", "/new")) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Only position delete files supported"); + } + + @Test + public void replacePathBoundsRewritesEqualBounds() { + String sourcePrefix = "/old/table"; + String targetPrefix = "/new/table"; + String dataFile = sourcePrefix + "/data/file.parquet"; + DeleteFile positionDelete = positionDeleteWithPathBounds(dataFile, dataFile); + + Metrics metrics = ContentFileUtil.replacePathBounds(positionDelete, sourcePrefix, targetPrefix); + + String expected = RewriteTablePathUtil.newPath(dataFile, sourcePrefix, targetPrefix); + assertThat(expected).isNotEqualTo(dataFile); + assertThat(decodePathBound(metrics.lowerBounds())).isEqualTo(expected); + assertThat(decodePathBound(metrics.upperBounds())).isEqualTo(expected); + // non-path bounds must be carried over untouched + assertThat(metrics.lowerBounds()).containsKey(POS_ID); + assertThat(metrics.upperBounds()).containsKey(POS_ID); + } + + @Test + public void replacePathBoundsClearsWhenUpperBoundMissing() { + Map<Integer, ByteBuffer> lowerBounds = Maps.newHashMap(); + lowerBounds.put(PATH_ID, pathBound("/old/table/data.parquet")); + lowerBounds.put(POS_ID, posBound(0L)); + Map<Integer, ByteBuffer> upperBounds = Maps.newHashMap(); + upperBounds.put(POS_ID, posBound(10L)); + DeleteFile positionDelete = positionDeleteWithBounds(lowerBounds, upperBounds); + + Metrics metrics = ContentFileUtil.replacePathBounds(positionDelete, "/old", "/new"); + + // upper path bound missing -> path bounds dropped, position bound preserved + assertThat(metrics.lowerBounds()).doesNotContainKey(PATH_ID).containsKey(POS_ID); + assertThat(metrics.upperBounds()).doesNotContainKey(PATH_ID).containsKey(POS_ID); + } + + @Test + public void replacePathBoundsClearsUnequalBounds() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/old/table/data-1.parquet", "/old/table/data-2.parquet"); + + Metrics metrics = ContentFileUtil.replacePathBounds(positionDelete, "/old", "/new"); + + // path bounds are dropped, but the position column bound is preserved + assertThat(metrics.lowerBounds()).doesNotContainKey(PATH_ID).containsKey(POS_ID); + assertThat(metrics.upperBounds()).doesNotContainKey(PATH_ID).containsKey(POS_ID); + } + + @Test + public void replacePathBoundsClearsMissingBounds() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + Metrics metrics = ContentFileUtil.replacePathBounds(positionDelete, "/old", "/new"); + + assertThat(metrics.lowerBounds()).isNull(); + assertThat(metrics.upperBounds()).isNull(); + } + + @Test + public void copyWithoutStatsDropsBounds() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/path/to/data.parquet", "/path/to/data.parquet"); + + DeleteFile copy = ContentFileUtil.copy(positionDelete, false, null); + + assertThat(copy.lowerBounds()).isNull(); + assertThat(copy.upperBounds()).isNull(); + } + + @Test + public void copyWithAllStatsKeepsBounds() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/path/to/data.parquet", "/path/to/data.parquet"); + + DeleteFile copy = ContentFileUtil.copy(positionDelete, true, null); + + assertThat(copy.lowerBounds()).containsKey(PATH_ID); + assertThat(copy.upperBounds()).containsKey(PATH_ID); + } + + @Test + public void copyWithRequestedStatsKeepsOnlyRequestedColumns() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/path/to/data.parquet", "/path/to/data.parquet"); + + DeleteFile kept = ContentFileUtil.copy(positionDelete, true, ImmutableSet.of(PATH_ID)); + assertThat(kept.lowerBounds()).containsKey(PATH_ID); + + DeleteFile dropped = ContentFileUtil.copy(positionDelete, true, ImmutableSet.of(POS_ID)); + assertThat(dropped.lowerBounds()).doesNotContainKey(PATH_ID); + } Review Comment: Do we need these three? `copy` only dispatches to `ContentFile.copy`, `copyWithStats`, or `copyWithoutStats`. `DeleteFileIndex` calls it with requested column IDs when it [loads delete manifests](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/main/java/org/apache/iceberg/DeleteFileIndex.java#L506-L512). [`testPositionDeleteDiscardMetrics`](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/test/java/org/apache/iceberg/DeleteFileIndexTestBase.java#L607) and [`testEqualityDeleteDiscardMetrics`](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/test/java/org/apache/iceberg/DeleteFileIndexTestBase.java#L646) check that only those stats survive. ########## core/src/test/java/org/apache/iceberg/util/TestContentFileUtil.java: ########## @@ -0,0 +1,358 @@ +/* + * 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 static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.nio.ByteBuffer; +import java.util.Map; +import org.apache.iceberg.DeleteFile; +import org.apache.iceberg.FileMetadata; +import org.apache.iceberg.MetadataColumns; +import org.apache.iceberg.Metrics; +import org.apache.iceberg.PartitionSpec; +import org.apache.iceberg.RewriteTablePathUtil; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList; +import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet; +import org.apache.iceberg.relocated.com.google.common.collect.Maps; +import org.apache.iceberg.types.Conversions; +import org.apache.iceberg.types.Type; +import org.junit.jupiter.api.Test; + +public class TestContentFileUtil { + + private static final PartitionSpec SPEC = PartitionSpec.unpartitioned(); + private static final int PATH_ID = MetadataColumns.DELETE_FILE_PATH.fieldId(); + private static final Type PATH_TYPE = MetadataColumns.DELETE_FILE_PATH.type(); + private static final int POS_ID = MetadataColumns.DELETE_FILE_POS.fieldId(); + + @Test + public void referencedDataFileReturnsNullForEqualityDeletes() { + DeleteFile equalityDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofEqualityDeletes(1) + .withPath("/path/to/equality-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(equalityDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsExplicitReference() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)) + .isEqualTo("/path/to/data.parquet"); + } + + @Test + public void referencedDataFileReturnsPathFromEqualBounds() { + String dataFile = "/path/to/data.parquet"; + DeleteFile positionDelete = positionDeleteWithPathBounds(dataFile, dataFile); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).hasToString(dataFile); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsDiffer() { + DeleteFile positionDelete = + positionDeleteWithPathBounds("/path/to/data-1.parquet", "/path/to/data-2.parquet"); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenBoundsMissing() { + DeleteFile positionDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileReturnsNullWhenUpperBoundMissing() { + // lower bound carries the path, but the upper bound for the path column is absent + Map<Integer, ByteBuffer> lowerBounds = Maps.newHashMap(); + lowerBounds.put(PATH_ID, pathBound("/path/to/data.parquet")); + Map<Integer, ByteBuffer> upperBounds = Maps.newHashMap(); + upperBounds.put(POS_ID, posBound(10L)); + DeleteFile positionDelete = positionDeleteWithBounds(lowerBounds, upperBounds); + + assertThat(ContentFileUtil.referencedDataFile(positionDelete)).isNull(); + } + + @Test + public void referencedDataFileLocation() { + DeleteFile withRef = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + DeleteFile withoutRef = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.referencedDataFileLocation(withRef)) + .isEqualTo("/path/to/data.parquet"); + assertThat(ContentFileUtil.referencedDataFileLocation(withoutRef)).isNull(); + } + + @Test + public void isFileScoped() { + DeleteFile fileScoped = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .withReferencedDataFile("/path/to/data.parquet") + .build(); + DeleteFile notFileScoped = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.isFileScoped(fileScoped)).isTrue(); + assertThat(ContentFileUtil.isFileScoped(notFileScoped)).isFalse(); + } + + @Test + public void isDV() { + assertThat(ContentFileUtil.isDV(dv())).isTrue(); + + DeleteFile parquetDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + assertThat(ContentFileUtil.isDV(parquetDelete)).isFalse(); + } + + @Test + public void containsSingleDV() { + DeleteFile dv = dv(); + DeleteFile parquetDelete = + FileMetadata.deleteFileBuilder(SPEC) + .ofPositionDeletes() + .withPath("/path/to/position-delete.parquet") + .withFileSizeInBytes(1024) + .withRecordCount(5) + .build(); + + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(dv))).isTrue(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of())).isFalse(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(dv(), dv()))).isFalse(); + assertThat(ContentFileUtil.containsSingleDV(ImmutableList.of(parquetDelete))).isFalse(); + } + + @Test + public void dvDesc() { + DeleteFile dv = dv(); + + assertThat(ContentFileUtil.dvDesc(dv)) + .contains("location=" + dv.location()) + .contains("offset=" + dv.contentOffset()) + .contains("length=" + dv.contentSizeInBytes()) + .contains("referencedDataFile=" + dv.referencedDataFile()); + } Review Comment: What do you think about dropping these tests, along with `referencedDataFileReturnsExplicitReference` at line 60? As I read them, none would fail on a change to logic in `ContentFileUtil`, and each one is more code to update when the `DeleteFile` builder changes. `referencedDataFileReturnsExplicitReference` reads back the value passed to `withReferencedDataFile`. `referencedDataFileLocation` and `isFileScoped` wrap `referencedDataFile` in a `toString()` and a `!= null` check. `isDV` is `format() == PUFFIN`, and `DeleteFileIndexTestBase` already checks it [directly](https://github.com/apache/iceberg/blob/39e35d1a87c0a6a9329d8ba74a45d587808598fa/core/src/test/java/org/apache/iceberg/DeleteFileIndexTestBase.java#L767-L773). `containsSingleDV` is a size check plus `isDV`, and `dvDesc` checks the output of `String.format`. -- 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]
