rdblue commented on code in PR #18108:
URL: https://github.com/apache/iceberg/pull/18108#discussion_r4148916813
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -1495,40 +1495,109 @@ public void
partitionFilterWithMultipleSpecs(FileFormat format) throws IOExcepti
.build();
Map<Integer, PartitionSpec> specsById =
ImmutableMap.of(idSpec.specId(), idSpec, dataSpec.specId(), dataSpec);
-
- PartitionData dataPartitionX = partition(dataSpec, "x");
- TrackedFile dataPartitionedFile =
- dataFileWithoutStats(
- "s3://bucket/table/data=x/file-c.parquet", dataSpec.specId(),
dataPartitionX);
-
- ManifestFile idPartitionedManifest =
- writeManifest(format, idSpec.partitionType(), ImmutableList.of(FILE_A,
FILE_B));
- ManifestFile dataPartitionedManifest =
- writeManifest(format, dataSpec.partitionType(), dataPartitionedFile);
-
- List<TrackedFile> files = Lists.newArrayList();
- files.addAll(
- read(
- V4ManifestReader.builder(idPartitionedManifest, IO, TABLE_SCHEMA,
specsById)
- .metricsConfig(METRICS_CONFIG)));
- files.addAll(
- read(
- V4ManifestReader.builder(dataPartitionedManifest, IO,
TABLE_SCHEMA, specsById)
- .metricsConfig(METRICS_CONFIG)));
-
Types.StructType unionType =
Partitioning.unionPartitionTypes(specsById.values());
+ int idPos = unionType.fields().indexOf(unionType.field("id"));
+ int dataPos = unionType.fields().indexOf(unionType.field("data"));
+
+ // a mixed-spec manifest stores every file's partition in the union type
+ PartitionData idOne = new PartitionData(unionType);
+ idOne.set(idPos, 1);
+ PartitionData idTwo = new PartitionData(unionType);
+ idTwo.set(idPos, 2);
+ PartitionData dataX = new PartitionData(unionType);
+ dataX.set(dataPos, "x");
+
+ TrackedFile idFileKept =
+ dataFileWithoutStats("s3://bucket/table/id=1/file-a.parquet",
idSpec.specId(), idOne);
+ TrackedFile idFilePruned =
+ dataFileWithoutStats("s3://bucket/table/id=2/file-b.parquet",
idSpec.specId(), idTwo);
+ TrackedFile dataFile =
+ dataFileWithoutStats("s3://bucket/table/data=x/file-c.parquet",
dataSpec.specId(), dataX);
- ManifestFile manifest = writeManifest(format, unionType, files);
+ ManifestFile manifest =
+ writeManifest(format, unionType, ImmutableList.of(idFileKept,
idFilePruned, dataFile));
V4ManifestReader.Builder builder =
V4ManifestReader.builder(manifest, IO, TABLE_SCHEMA, specsById)
.filter(Expressions.equal("id", 1))
.metricsConfig(METRICS_CONFIG);
- // the comparator is built for ID partitioning, so only check the location
assertThat(read(builder))
.extracting(TrackedFile::location)
- .containsExactlyInAnyOrder(FILE_A.location(),
dataPartitionedFile.location());
+ .containsExactlyInAnyOrder(idFileKept.location(), dataFile.location());
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void narrowPartitionProjectionReadsFullUnionTuple(FileFormat format)
throws IOException {
+ PartitionSpec idSpec =
+ PartitionSpec.builderFor(TABLE_SCHEMA)
+ .withSpecId(1)
+ .add(1, 1000, "id", Transforms.identity())
+ .build();
+ PartitionSpec dataSpec =
+ PartitionSpec.builderFor(TABLE_SCHEMA)
+ .withSpecId(2)
+ .add(2, 1001, "data", Transforms.identity())
+ .build();
+ Map<Integer, PartitionSpec> specsById =
+ ImmutableMap.of(idSpec.specId(), idSpec, dataSpec.specId(), dataSpec);
+ Types.StructType unionType =
Partitioning.unionPartitionTypes(specsById.values());
+
+ PartitionData unionPartition = new PartitionData(unionType);
+ unionPartition.set(unionType.fields().indexOf(unionType.field("data")),
"x");
+ TrackedFile file =
+ dataFileWithoutStats(
+ "s3://bucket/table/data=x/file.parquet", dataSpec.specId(),
unionPartition);
+ ManifestFile manifest = writeManifest(format, unionType,
ImmutableList.of(file));
+
+ TrackedFile actual =
+ readOne(
+ V4ManifestReader.builder(manifest, IO, TABLE_SCHEMA, specsById)
+ .metricsConfig(METRICS_CONFIG)
+ .select("partition.id"));
+ assertThat(actual.partition().get(0, CharSequence.class)).hasToString("x");
+ }
+
+ @ParameterizedTest
+ @FieldSource("MANIFEST_FORMATS")
+ public void unknownSpecPartitionIsNotProjected(FileFormat format) throws
IOException {
Review Comment:
I don't think this is quite the right behavior. If the spec is unknown then
`partition` cannot fulfill its contract, which is to return the original
partition tuple with the spec's output type. That type is unknown and we can't
just substitute the union type.
I think the choice is between returning null and failing. Failing is safer,
but returning null should have mostly the same effect because a file should not
have a `null` partition when it has a non-null spec ID.
--
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]
