stevenzwu commented on code in PR #17434:
URL: https://github.com/apache/iceberg/pull/17434#discussion_r3716274855
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -723,38 +731,203 @@ public void unknownManifestFormatThrows() throws
IOException {
InputFile badFile =
fileIO.newInputFile(tempDir.resolve("manifest-" + System.nanoTime() +
".txt").toString());
- try (V4ManifestReader reader = V4ManifestReader.builder(badFile,
UNPARTITIONED_SPECS).build()) {
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(badFile, UNPARTITIONED_SPECS,
TABLE_LOCATION).build()) {
assertThatThrownBy(reader::iterator)
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Cannot determine format of manifest");
}
}
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesRelativeDataFileLocation(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA);
Review Comment:
the file name probably shouldn't hardcoded to `.parquet`?
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -723,38 +731,203 @@ public void unknownManifestFormatThrows() throws
IOException {
InputFile badFile =
fileIO.newInputFile(tempDir.resolve("manifest-" + System.nanoTime() +
".txt").toString());
- try (V4ManifestReader reader = V4ManifestReader.builder(badFile,
UNPARTITIONED_SPECS).build()) {
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(badFile, UNPARTITIONED_SPECS,
TABLE_LOCATION).build()) {
assertThatThrownBy(reader::iterator)
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Cannot determine format of manifest");
}
}
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesRelativeDataFileLocation(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA);
+ verifyLocation(format, file, TABLE_LOCATION + "/data/00000-0.parquet");
Review Comment:
nit: use the `LocationUtil.resolveLocation` here?
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -723,38 +731,203 @@ public void unknownManifestFormatThrows() throws
IOException {
InputFile badFile =
fileIO.newInputFile(tempDir.resolve("manifest-" + System.nanoTime() +
".txt").toString());
- try (V4ManifestReader reader = V4ManifestReader.builder(badFile,
UNPARTITIONED_SPECS).build()) {
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(badFile, UNPARTITIONED_SPECS,
TABLE_LOCATION).build()) {
assertThatThrownBy(reader::iterator)
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Cannot determine format of manifest");
}
}
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesRelativeDataFileLocation(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA);
+ verifyLocation(format, file, TABLE_LOCATION + "/data/00000-0.parquet");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void absoluteDataFileLocationIsUnchanged(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("hdfs://wh/db/table/data/00000-0.parquet",
EMPTY_PARTITION_DATA);
+ verifyLocation(format, file, "hdfs://wh/db/table/data/00000-0.parquet");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void preservesNonStandardDataFileLocation(FileFormat format) throws
IOException {
+ // a leading / or // has no URI scheme, so it is treated as relative and
joined to the table
+ // location; the reader does not special-case authority-style or
root-absolute paths
+ verifyLocation(
+ format,
+ dataFile("/data/00000-0.parquet", EMPTY_PARTITION_DATA),
+ TABLE_LOCATION + "//data/00000-0.parquet");
+ verifyLocation(
+ format,
+ dataFile("//data/00000-0.parquet", EMPTY_PARTITION_DATA),
+ TABLE_LOCATION + "///data/00000-0.parquet");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesRelativeDeletionVectorLocation(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA,
dv("data/dv.puffin"));
+
+ InputFile manifest = writeManifest(format, EMPTY_PARTITION,
ImmutableList.of(file));
+
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS,
TABLE_LOCATION).build()) {
+ TrackedFile actual = Iterables.getOnlyElement(reader);
+ assertThat(actual.location()).isEqualTo(TABLE_LOCATION +
"/data/00000-0.parquet");
+ assertThat(actual.deletionVector().location()).isEqualTo(TABLE_LOCATION
+ "/data/dv.puffin");
+ }
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesLeafManifestLocation(FileFormat format) throws
IOException {
+ TrackedFile leaf = manifestRef(FileContent.DATA_MANIFEST,
"metadata/leaf.avro");
+ verifyLocation(format, leaf, TABLE_LOCATION + "/metadata/leaf.avro");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesDataFileAndDvSchemesIndependently(FileFormat format)
throws IOException {
+ // absolute data file paired with a relative DV, and relative data file
paired with an absolute
+ // DV: each location's scheme is evaluated on its own
+ TrackedFile absoluteFileRelativeDv =
+ dataFile("s3://other/abs.parquet", EMPTY_PARTITION_DATA,
dv("data/dv.puffin"));
+ TrackedFile relativeFileAbsoluteDv =
+ dataFile("data/rel.parquet", EMPTY_PARTITION_DATA,
dv("s3://other/abs-dv.puffin"));
+
+ InputFile manifest =
+ writeManifest(
+ format,
+ EMPTY_PARTITION,
+ ImmutableList.of(absoluteFileRelativeDv, relativeFileAbsoluteDv));
+
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS,
TABLE_LOCATION).build()) {
+ List<TrackedFile> actual = Lists.newArrayList(reader);
+ assertThat(actual.get(0).location()).isEqualTo("s3://other/abs.parquet");
+ assertThat(actual.get(0).deletionVector().location())
+ .isEqualTo(TABLE_LOCATION + "/data/dv.puffin");
+ assertThat(actual.get(1).location()).isEqualTo(TABLE_LOCATION +
"/data/rel.parquet");
+
assertThat(actual.get(1).deletionVector().location()).isEqualTo("s3://other/abs-dv.puffin");
+ }
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void stripsTrailingSlashFromTableLocation(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA);
+
+ InputFile manifest = writeManifest(format, EMPTY_PARTITION,
ImmutableList.of(file));
+
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS, TABLE_LOCATION
+ "/").build()) {
+ TrackedFile actual = Iterables.getOnlyElement(reader);
+ assertThat(actual.location()).isEqualTo(TABLE_LOCATION +
"/data/00000-0.parquet");
+ }
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolutionSkippedWhenLocationNotProjected(FileFormat format)
throws IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA);
+
+ InputFile manifest = writeManifest(format, EMPTY_PARTITION,
ImmutableList.of(file));
+
+ // location is not projected, so there is nothing to resolve even though
it is relative
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS, TABLE_LOCATION)
+ .select("tracking.status")
+ .build()) {
+ TrackedFile actual = Iterables.getOnlyElement(reader);
+ assertThat(actual.location()).isNull();
+ }
+ }
+
@Test
public void invalidBuilderArguments() {
InputFile manifest =
fileIO.newInputFile(tempDir.resolve("manifest.avro").toString());
- assertThatThrownBy(() -> V4ManifestReader.builder(manifest,
UNPARTITIONED_SPECS).filter(null))
+ assertThatThrownBy(
+ () ->
+ V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS,
TABLE_LOCATION)
+ .filter(null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid filter: null");
assertThatThrownBy(
- () -> V4ManifestReader.builder(manifest,
UNPARTITIONED_SPECS).scanMetrics(null))
+ () ->
+ V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS,
TABLE_LOCATION)
+ .scanMetrics(null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid scan metrics: null");
assertThatThrownBy(
() ->
- V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS)
+ V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS,
TABLE_LOCATION)
.select((Collection<String>) null))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid columns: null");
+
+ assertThatThrownBy(() -> V4ManifestReader.builder(manifest,
UNPARTITIONED_SPECS, null))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessage("Invalid table location: null");
+ }
+
+ // the location a relative fixture resolves to once read against
TABLE_LOCATION
+ private static String resolved(TrackedFile file) {
+ return LocationUtil.resolveLocation(TABLE_LOCATION, file.location());
+ }
+
+ // writes a single tracked file, reads it back against TABLE_LOCATION, and
checks its location
+ private void verifyLocation(FileFormat format, TrackedFile file, String
expectedLocation)
Review Comment:
`verifyLocationAfterWriteReadRoundTrip`?
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -723,38 +731,203 @@ public void unknownManifestFormatThrows() throws
IOException {
InputFile badFile =
fileIO.newInputFile(tempDir.resolve("manifest-" + System.nanoTime() +
".txt").toString());
- try (V4ManifestReader reader = V4ManifestReader.builder(badFile,
UNPARTITIONED_SPECS).build()) {
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(badFile, UNPARTITIONED_SPECS,
TABLE_LOCATION).build()) {
assertThatThrownBy(reader::iterator)
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Cannot determine format of manifest");
}
}
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesRelativeDataFileLocation(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA);
+ verifyLocation(format, file, TABLE_LOCATION + "/data/00000-0.parquet");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void absoluteDataFileLocationIsUnchanged(FileFormat format) throws
IOException {
Review Comment:
do we need to test all the resolution scenario here? `TestLocationUtil`
should cover all these scenarios already.
This applies to the method below too.
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -723,38 +731,203 @@ public void unknownManifestFormatThrows() throws
IOException {
InputFile badFile =
fileIO.newInputFile(tempDir.resolve("manifest-" + System.nanoTime() +
".txt").toString());
- try (V4ManifestReader reader = V4ManifestReader.builder(badFile,
UNPARTITIONED_SPECS).build()) {
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(badFile, UNPARTITIONED_SPECS,
TABLE_LOCATION).build()) {
assertThatThrownBy(reader::iterator)
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("Cannot determine format of manifest");
}
}
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesRelativeDataFileLocation(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA);
+ verifyLocation(format, file, TABLE_LOCATION + "/data/00000-0.parquet");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void absoluteDataFileLocationIsUnchanged(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("hdfs://wh/db/table/data/00000-0.parquet",
EMPTY_PARTITION_DATA);
+ verifyLocation(format, file, "hdfs://wh/db/table/data/00000-0.parquet");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void preservesNonStandardDataFileLocation(FileFormat format) throws
IOException {
+ // a leading / or // has no URI scheme, so it is treated as relative and
joined to the table
+ // location; the reader does not special-case authority-style or
root-absolute paths
+ verifyLocation(
+ format,
+ dataFile("/data/00000-0.parquet", EMPTY_PARTITION_DATA),
+ TABLE_LOCATION + "//data/00000-0.parquet");
+ verifyLocation(
+ format,
+ dataFile("//data/00000-0.parquet", EMPTY_PARTITION_DATA),
+ TABLE_LOCATION + "///data/00000-0.parquet");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesRelativeDeletionVectorLocation(FileFormat format) throws
IOException {
+ TrackedFile file = dataFile("data/00000-0.parquet", EMPTY_PARTITION_DATA,
dv("data/dv.puffin"));
+
+ InputFile manifest = writeManifest(format, EMPTY_PARTITION,
ImmutableList.of(file));
+
+ try (V4ManifestReader reader =
+ V4ManifestReader.builder(manifest, UNPARTITIONED_SPECS,
TABLE_LOCATION).build()) {
+ TrackedFile actual = Iterables.getOnlyElement(reader);
+ assertThat(actual.location()).isEqualTo(TABLE_LOCATION +
"/data/00000-0.parquet");
+ assertThat(actual.deletionVector().location()).isEqualTo(TABLE_LOCATION
+ "/data/dv.puffin");
+ }
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesLeafManifestLocation(FileFormat format) throws
IOException {
+ TrackedFile leaf = manifestRef(FileContent.DATA_MANIFEST,
"metadata/leaf.avro");
+ verifyLocation(format, leaf, TABLE_LOCATION + "/metadata/leaf.avro");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void resolvesDataFileAndDvSchemesIndependently(FileFormat format)
throws IOException {
Review Comment:
`SchemeIndependently` is not very clear to me. maybe call this
`mixedAbsoluteAndRelativePathWithDataFileAndDV`?
--
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]
