You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/10/28 13:00:30 UTC
[GitHub] [iceberg] ConeyLiu opened a new pull request #3411: Fixes read metadata failed after dropped partition
ConeyLiu opened a new pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411
This patch fixes the reading metadata table(files, manifests, all_manifests) that failed after dropped partition.
Closes #3374
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813396462
##########
File path: core/src/test/java/org/apache/iceberg/TestPartitioning.java
##########
@@ -140,9 +140,10 @@ public void testPartitionTypeWithAddingBackSamePartitionFieldInV1Table() {
.addField("data")
.commit();
- // in v1, we use void transforms instead of dropping partition fields
+ // in v1, we use void transforms instead of dropping partition fields.
+ // We restored the void transforms with the original dropped partition fields.
Review comment:
I'm not sure line 144 here makes sense to me, I think this is fine without the comment. In fact I think the expected type makes more sense now than it previously did.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814589358
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .appendFile(FILE_B)
+ .commit();
+
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
+ table.refresh();
+
+ Table manifestsTable = new ManifestsTable(table.ops(), table);
+ TableScan scan = manifestsTable.newScan()
+ .filter(Expressions.lessThan("length", 10000L));
Review comment:
It could be removed. The test is adjusted from `testManifestsTableAlwaysIgnoresResiduals`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r824305576
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I'm also up for solution 2, where we rename just the columns from older specs. That sounds like a reasonable solution.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813265716
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .appendFile(FILE_B)
+ .commit();
+
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
+ table.refresh();
+
+ Table manifestsTable = new ManifestsTable(table.ops(), table);
+ TableScan scan = manifestsTable.newScan()
+ .filter(Expressions.lessThan("length", 10000L));
Review comment:
Why are we filtering?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu edited a comment on pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu edited a comment on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-953822189
Hi @RussellSpitzer @rdblue @openinx @jackye1995, could you help to review this? Thanks a lot.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825483801
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -222,27 +224,41 @@ public static StructType partitionType(Table table) {
for (PartitionField field : spec.fields()) {
int fieldId = field.fieldId();
+ NestedField structField = spec.partitionType().field(fieldId);
+
Review comment:
Nit: no need for a newline between assignment statements.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814848640
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
Review comment:
ManfiestEntries and partitions should be slightly different right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-953822189
Hi @RussellSpitzer @rdblue @openinx @jackye1995, please help to review this. Thanks a lot.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1059634289
Sorry for the delay on my side. This PR is the next one I'll look into. I took a quick look and I think I remember the context.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825485003
##########
File path: core/src/main/java/org/apache/iceberg/ManifestsTable.java
##########
@@ -73,10 +71,15 @@ MetadataTableType metadataTableType() {
protected DataTask task(TableScan scan) {
TableOperations ops = operations();
String location = scan.snapshot().manifestListLocation();
+ Map<Integer, PartitionSpec> specs = table().specs();
Review comment:
Can you update this to copy the map into a new `HashMap`? Not all maps are serializable with Kryo, so we should make sure it is created by `Maps.newHashMap` to be safe.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1080504744
Rebased, thanks for the reminder. @aokolnychyi
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814576895
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -134,14 +136,14 @@ public long targetSplitSize() {
.withRecordCount(1)
.withFormat(FileFormat.AVRO)
.build();
- return new ManifestListReadTask(ops.io(), schema(), table().spec(), new BaseFileScanTask(
+ return new ManifestListReadTask(ops.io(), schema(), specs, new BaseFileScanTask(
manifestListAsDataFile, null,
schemaString, specString, residuals));
} else {
return StaticDataTask.of(
ops.io().newInputFile(ops.current().metadataFileLocation()),
MANIFEST_FILE_SCHEMA, schema(), snap.allManifests(),
- manifest -> ManifestsTable.manifestFileToRow(table().spec(), manifest)
+ manifest -> ManifestsTable.manifestFileToRow(specs.get(manifest.partitionSpecId()), manifest)
Review comment:
`Partitioning.partitionType` builds a common partition type, here we need a `PartitionSpec` argument.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814612980
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
+ if (Transforms.alwaysNull().equals(field.transform())) {
+ fieldToIndex.put(fieldId, structFields.size() - 1);
+ }
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
+
+ if (Transforms.alwaysNull().equals(existingField.transform()) &&
+ !Transforms.alwaysNull().equals(field.transform())) {
+ fieldMap.put(fieldId, field);
+ NestedField structField = spec.partitionType().field(fieldId);
+ structFields.set(fieldToIndex.get(fieldId), structField);
Review comment:
```java
fieldMap.put(fieldId, field);
NestedField previousTransform = spec.partitionType().field(fieldId);
int voidTransformIndex = structFields.indexOf(existingField); // here, we need a `NestedField` instance not a `PartitionField` instance, and it seems that we could not get the previous `PartitionField` because we are in new spec.
structFields.set(voidTransformIndex, previousTransform)
```
Please see the above inline comments.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813394547
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
Review comment:
I think for completeness sake we should also have tests for the other Partition reading metadata tables. (Files, Manfiests, ManifestEntries) or what not
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814584886
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
Review comment:
It should be only in V1. I think it should be OK that the tests covered v2 as well, that's right?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r821571379
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -211,6 +211,8 @@ public static StructType partitionType(Table table) {
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
List<NestedField> structFields = Lists.newArrayList();
+ // Mapping from a PartitionField with void transform to the index in structFields.
Review comment:
Updated. Thanks for the suggestion.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r823185064
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I think Field_id Suffix seems very reasonable to me.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szlta commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r822676643
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
@aokolnychyi I think you're referring to what's happening in V1 tables. For those the spec is ever-growing in a way that no partition fields/transforms are removed, but rather converted to void.
The rename logic is there: https://github.com/apache/iceberg/blob/master/core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Ficeberg%2FBaseUpdatePartitionSpec.java#L179
but I think this is not used for V2 tables, as per https://github.com/apache/iceberg/blob/master/core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Ficeberg%2FBaseUpdatePartitionSpec.java#L261-L268
Since in V2, specs don't retain old deleted partition fields this rename is not required for normal operations.
The problem I'm describing only affects the metadata table queries, because for V2, due to the lack of above renames, Partitioning.partitionType() collects all partition fields from all previous specs too. With the lack of renames this can result in the same field name being present multiple times, and cause the PartitionsTable's (or DataFilesTable's) schema to be failed to get constructed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825485087
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -118,6 +119,7 @@ protected String tableType() {
boolean ignoreResiduals, boolean caseSensitive, boolean colStats) {
String schemaString = SchemaParser.toJson(schema());
String specString = PartitionSpecParser.toJson(PartitionSpec.unpartitioned());
+ Map<Integer, PartitionSpec> specs = table().specs();
Review comment:
Similar to the `ManifestsTable`, I think this needs to copy the specs into a `newHashMap`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r823151850
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I guess you are right, @szlta. Here is an example to reproduce.
```
PartitionSpec initialSpec = PartitionSpec.builderFor(SCHEMA)
.identity("data")
.build();
TestTables.TestTable table = TestTables.create(tableDir, "test", SCHEMA, initialSpec, V2_FORMAT_VERSION);
table.updateSpec()
.removeField("data")
.commit();
table.updateSpec()
.addField("data")
.addField("id")
.commit();
struct<1000: data: optional string, 1001: data: optional string, 1002: id: optional int>
```
While it would be great to update the logic that evolves the spec, I think we have to adapt the method that builds a common representation too. Maintaining a set of used names and appending a suffix of the field ID sounds like a reasonable approach.
Thoughts, @rdblue @RussellSpitzer @szehon-ho @flyrain?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r823151850
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I guess you are right, @szlta. Here is an example to reproduce.
```
PartitionSpec initialSpec = PartitionSpec.builderFor(SCHEMA)
.identity("data")
.build();
TestTables.TestTable table = TestTables.create(tableDir, "test", SCHEMA, initialSpec, V2_FORMAT_VERSION);
table.updateSpec()
.removeField("data")
.commit();
table.updateSpec()
.addField("data")
.addField("id")
.commit();
struct<1000: data: optional string, 1001: data: optional string, 1002: id: optional int>
```
While it would be great to update the logic that evolves the spec, I think we have to adapt the method that builds a common representation too. Otherwise, existing tables may be forever broken. Maintaining a set of used names and appending a suffix of the field ID sounds like a reasonable approach.
Thoughts, @rdblue @RussellSpitzer @szehon-ho @flyrain?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r738514115
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
Review comment:
Could you explain these changes? I'm not sure I understand the logic here. If the field is always null (void) then we place it in a mapping of field id to the last index of the structfields?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1080805318
Also thanks to all of the reviewers who took their time to check on this PR, I know this was a tricky issue and I'm glad we had so many eyes on the issue.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813260910
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -134,14 +136,14 @@ public long targetSplitSize() {
.withRecordCount(1)
.withFormat(FileFormat.AVRO)
.build();
- return new ManifestListReadTask(ops.io(), schema(), table().spec(), new BaseFileScanTask(
+ return new ManifestListReadTask(ops.io(), schema(), specs, new BaseFileScanTask(
manifestListAsDataFile, null,
schemaString, specString, residuals));
} else {
return StaticDataTask.of(
ops.io().newInputFile(ops.current().metadataFileLocation()),
MANIFEST_FILE_SCHEMA, schema(), snap.allManifests(),
- manifest -> ManifestsTable.manifestFileToRow(table().spec(), manifest)
+ manifest -> ManifestsTable.manifestFileToRow(specs.get(manifest.partitionSpecId()), manifest)
Review comment:
Is this change needed, shouldn't we be using the common spec from table built in Partitioning?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813393371
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
+ if (Transforms.alwaysNull().equals(field.transform())) {
+ fieldToIndex.put(fieldId, structFields.size() - 1);
+ }
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
+
+ if (Transforms.alwaysNull().equals(existingField.transform()) &&
+ !Transforms.alwaysNull().equals(field.transform())) {
+ fieldMap.put(fieldId, field);
+ NestedField structField = spec.partitionType().field(fieldId);
+ structFields.set(fieldToIndex.get(fieldId), structField);
Review comment:
I think this may be easier (and we wouldn't need to build up the index map) to just do
```java
fieldMap.put(fieldId, field);
NestedField previousTransform = spec.partitionType().field(fieldId);
voidTransformIndex = structFields.indexOf(existingField)
structFields.set(voidTransformIndex, previousTransform)
```
I recommend this because I think it more clearly expresses what we are trying to do here. When initially looked at the code It wasn't clear to me that the fieldToIndex was solely for looking up old void transforms for this edge case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813265025
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
Review comment:
I think the key part of this test is we should be checking a dropped V1 Field whose return type changes when dropped. As is this test I believe would always pass on a V2 table even without this patch.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r815213896
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
+ if (Transforms.alwaysNull().equals(field.transform())) {
+ fieldToIndex.put(fieldId, structFields.size() - 1);
+ }
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
+
+ if (Transforms.alwaysNull().equals(existingField.transform()) &&
+ !Transforms.alwaysNull().equals(field.transform())) {
+ fieldMap.put(fieldId, field);
+ NestedField structField = spec.partitionType().field(fieldId);
+ structFields.set(fieldToIndex.get(fieldId), structField);
Review comment:
ok got it, we need the "spec.partitionType()" of the spec the partition field was created with, and at this point we have lost that reference
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813393371
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
+ if (Transforms.alwaysNull().equals(field.transform())) {
+ fieldToIndex.put(fieldId, structFields.size() - 1);
+ }
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
+
+ if (Transforms.alwaysNull().equals(existingField.transform()) &&
+ !Transforms.alwaysNull().equals(field.transform())) {
+ fieldMap.put(fieldId, field);
+ NestedField structField = spec.partitionType().field(fieldId);
+ structFields.set(fieldToIndex.get(fieldId), structField);
Review comment:
I think this may be easier (and we wouldn't need to build up the index map) to just do
```java
fieldMap.put(fieldId, field);
NestedField previousTransform = spec.partitionType().field(fieldId);
int voidTransformIndex = structFields.indexOf(existingField)
structFields.set(voidTransformIndex, previousTransform)
```
I recommend this because I think it more clearly expresses what we are trying to do here. When initially looked at the code It wasn't clear to me that the fieldToIndex was solely for looking up old void transforms for this edge case.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814589741
##########
File path: core/src/test/java/org/apache/iceberg/TestPartitioning.java
##########
@@ -140,9 +140,10 @@ public void testPartitionTypeWithAddingBackSamePartitionFieldInV1Table() {
.addField("data")
.commit();
- // in v1, we use void transforms instead of dropping partition fields
+ // in v1, we use void transforms instead of dropping partition fields.
+ // We restored the void transforms with the original dropped partition fields.
Review comment:
OK, remove it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814850412
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
Review comment:
Yeah I just wanted to make sure we note that this test was added to check the transformation of a partition column's type. Like we are actually checking something a little more specific that dropping a partition, since if "data" was an int there wouldn't be a bug.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r817514806
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
+ if (Transforms.alwaysNull().equals(field.transform())) {
+ fieldToIndex.put(fieldId, structFields.size() - 1);
+ }
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
+
+ if (Transforms.alwaysNull().equals(existingField.transform()) &&
+ !Transforms.alwaysNull().equals(field.transform())) {
+ fieldMap.put(fieldId, field);
+ NestedField structField = spec.partitionType().field(fieldId);
+ structFields.set(fieldToIndex.get(fieldId), structField);
Review comment:
Yeah, that's right.
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
Review comment:
Added
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r817515488
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -66,6 +70,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .appendFile(FILE_B)
+ .commit();
+
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
Review comment:
updated
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r738514115
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
Review comment:
Could you explain these changes? I'm not sure I understand the logic here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szlta commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r821845093
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I have run into similar issues, and I think this will help resolve the type change of columns in older specs.
Another thing I have seen and is probably still a problem, is that this method may return a column name multiple times. Consider the following:
Table schema: a int, b date, c date
spec0: year(b), a
spec1: a
spec2: year(b), year(c)
then the result is something like this: 1000: b_year int, 1001: a int, 1002: b_year int, 1003: c_year int
Further down the line when we construct a Schema object, we will have a failure due to b_year name being present in two fields (1000, 1002).
How should this case be handled? Maybe appending _r<fieldId> to each column name?
cc: @RussellSpitzer , @aokolnychyi
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1059640267
Okay, I think I got the issue. Essentially, we don't know the correct type produced by Void transforms that we assign in v1 tables when a partition field is dropped. We assume the output type matches the source field type but it is not always the case. Let me take a look at the solution on Monday.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szlta commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r823692312
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
What about partition transforms where the transform itself could change, but the field name remains the same?
E.g.: `bucket(data, 10)` would be `data_bucket`, and after dropping this from spec and re-adding with a new spec `bucket(data, 8), id`, then we would have `1000: data_bucket, 1001: data_bucket, 1002: id `where the two `data_bucket` fields describe different things. I guess this is similar for truncate.
A partition in the old spec where `data_bucket = 0` would be different from a partition in the new spec where `data_bucket = 0`
I agree with @rdblue that it's weird to have `category_r1003` in queries, but if we want to avoid it, I see two ways of proceeding from metadata query perspective:
A metadata table should..
1. ..either give back partition information as per the latest spec only..
2. ..or we could combine the data returned for similarly named partition fields cascaded into just one field, but extend such tables with `spec_id` information in these cases.
So as per above example use case the `partitions` table would look like this for the two cases (with 1 partition in each spec):
```
1:
+---------------------------------+--------------------+------------------+
| test.partition | test.record_count | test.file_count |
+---------------------------------+--------------------+------------------+
| {data_bucket : 0, id : 1} | 1 | 1 |
+---------------------------------+--------------------+------------------+
2:
+---------------------------------+--------------------+------------------+--------------+
| test.partition | test.record_count | test.file_count | test.spec_id |
+---------------------------------+--------------------+------------------+--------------+
| {data_bucket : 0, id : null} | 1 | 1 | 0 |
| {data_bucket : 0, id : 1} | 1 | 1 | 1 |
+---------------------------------+--------------------+------------------+--------------+
```
Note this is how it would be for V2 tables. For V1, due to the renaming we're already doing, the renamed fields would be present too (unless we aim at changing that too):
```
1:
+---------------------------------+--------------------+------------------+
| test.partition | test.record_count | test.file_count |
+---------------------------------+--------------------+------------------+
| {data_bucket : 0, id : 1} | 1 | 1 |
+---------------------------------+--------------------+------------------+
2:
+----------------------------------------------------------+--------------------+------------------+--------------+
| test.partition | test.record_count | test.file_count | test.spec_id |
+----------------------------------------------------------+--------------------+------------------+--------------+
| {data_bucket_1000 : 0, data_bucket : null, id : null} | 1 | 1 | 0 |
| {data_bucket_1000 : null, data_bucket: 0, id : 1} | 1 | 1 | 1 |
+----------------------------------------------------------+--------------------+------------------+--------------+
```
I'm personally more for solution 2, where we don't have to omit the old partitions but at the same time we get a nice and coherent partition info result. It's kind of in league what I'm proposing in issue #4292 (we could also continue the discussion of this problem there, I didn't mean to hijack @ConeyLiu 's PR like this)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1066094177
> @ConeyLiu, can you update the description? It is really helpful to not only say what you're trying to fix, but also to describe the approach and why the changes are needed. Thanks!
Updated.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813388654
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
+ if (Transforms.alwaysNull().equals(field.transform())) {
+ fieldToIndex.put(fieldId, structFields.size() - 1);
+ }
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
+
+ if (Transforms.alwaysNull().equals(existingField.transform()) &&
Review comment:
I think we should add a comment here,
"Void Transforms always revert back to the Type of the original column being transformed. This will lead to type errors when analyzing partition fields written for earlier files with a different type. To avoid this we always restore NullTransforms to their previous non-null state."
Or something like that?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1056700258
@RussellSpitzer, I am sorry for the late update. Thanks for the review. Comments have been addressed. Pls take another look when you are free.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r823158313
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
Looks like we are using field IDs while projecting values from the common representation in Spark.
At least, that part is working.
```
protected Map<Integer, StructProjection> buildPartitionProjections(Types.StructType partitionType,
Map<Integer, PartitionSpec> specs) {
Map<Integer, StructProjection> partitionProjections = Maps.newHashMap();
specs.forEach((specID, spec) ->
partitionProjections.put(specID, StructProjection.create(partitionType, spec.partitionType()))
);
return partitionProjections;
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825534705
##########
File path: core/src/main/java/org/apache/iceberg/ManifestsTable.java
##########
@@ -73,10 +71,15 @@ MetadataTableType metadataTableType() {
protected DataTask task(TableScan scan) {
TableOperations ops = operations();
String location = scan.snapshot().manifestListLocation();
+ Map<Integer, PartitionSpec> specs = table().specs();
Review comment:
updated
##########
File path: core/src/main/java/org/apache/iceberg/AllManifestsTable.java
##########
@@ -118,6 +119,7 @@ protected String tableType() {
boolean ignoreResiduals, boolean caseSensitive, boolean colStats) {
String schemaString = SchemaParser.toJson(schema());
String specString = PartitionSpecParser.toJson(PartitionSpec.unpartitioned());
+ Map<Integer, PartitionSpec> specs = table().specs();
Review comment:
updated
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825484541
##########
File path: core/src/test/java/org/apache/iceberg/TestPartitioning.java
##########
@@ -140,7 +140,7 @@ public void testPartitionTypeWithAddingBackSamePartitionFieldInV1Table() {
.addField("data")
.commit();
- // in v1, we use void transforms instead of dropping partition fields
+ // in v1, we use void transforms instead of dropping partition fields.
Review comment:
Please remove this change since it is non-functional. Also, we don't require punctuation to end comments.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r821959509
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
@szlta, Iceberg should rename older columns when it updates the spec to avoid conflicts. That's why we have to take the latest column names. I think we should have some tests in `TestPartitioning`. Could you check, @szlta?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r821570058
##########
File path: core/src/test/java/org/apache/iceberg/TestPartitioning.java
##########
@@ -140,9 +140,9 @@ public void testPartitionTypeWithAddingBackSamePartitionFieldInV1Table() {
.addField("data")
.commit();
- // in v1, we use void transforms instead of dropping partition fields
+ // in v1, we use void transforms instead of dropping partition fields.
StructType expectedType = StructType.of(
- NestedField.optional(1000, "data_1000", Types.StringType.get()),
+ NestedField.optional(1000, "data", Types.StringType.get()),
Review comment:
reverted
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825534615
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -222,27 +224,41 @@ public static StructType partitionType(Table table) {
for (PartitionField field : spec.fields()) {
int fieldId = field.fieldId();
+ NestedField structField = spec.partitionType().field(fieldId);
+
Review comment:
removed
##########
File path: core/src/test/java/org/apache/iceberg/TestPartitioning.java
##########
@@ -140,7 +140,7 @@ public void testPartitionTypeWithAddingBackSamePartitionFieldInV1Table() {
.addField("data")
.commit();
- // in v1, we use void transforms instead of dropping partition fields
+ // in v1, we use void transforms instead of dropping partition fields.
Review comment:
reverted
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1080804203
Thanks @ConeyLiu !
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813394547
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
Review comment:
I think for completeness sake we should also have tests for the other Partition reading metadata tables. (Files, ManifestEntries, Partitions) or what not
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r815216063
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -66,6 +70,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
+ .appendFile(FILE_B)
+ .commit();
+
+ table.updateSpec().removeField(Expressions.bucket("data", 16)).commit();
Review comment:
Could we add a test which drops and adds partitioning more than once? I just want to have confidence for something like
Int -> Void<String> -> Int -> Void<String> -> Truncate<String>
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r817515358
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -211,6 +211,7 @@ public static StructType partitionType(Table table) {
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Integer> fieldToIndex = Maps.newHashMap();
Review comment:
Updated the name, and left comments here.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814588606
##########
File path: core/src/test/java/org/apache/iceberg/TestMetadataTableScans.java
##########
@@ -63,6 +67,28 @@ private void preparePartitionedTable() {
.commit();
}
+ @Test
+ public void testManifestsTableWithDroppedPartition() throws IOException {
+ table.newFastAppend()
+ .appendFile(FILE_A)
Review comment:
Should the following tests(`testAllManifestsTableWithDroppedPartition` and `testFilesTableScanWithDroppedPartition`) have covered the case ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r821254517
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -211,6 +211,8 @@ public static StructType partitionType(Table table) {
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
List<NestedField> structFields = Lists.newArrayList();
+ // Mapping from a PartitionField with void transform to the index in structFields.
Review comment:
It is a little bit hard to follow these changes and they also break the logic for inheriting updated column names.
What about something like this?
```
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
Map<Integer, Type> typeMap = Maps.newHashMap();
Map<Integer, String> nameMap = Maps.newHashMap();
// sort the spec IDs in descending order to pick up the most recent field names
List<Integer> specIds = table.specs().keySet().stream()
.sorted(Collections.reverseOrder())
.collect(Collectors.toList());
for (Integer specId : specIds) {
PartitionSpec spec = table.specs().get(specId);
for (PartitionField field : spec.fields()) {
int fieldId = field.fieldId();
NestedField structField = spec.partitionType().field(fieldId);
PartitionField existingField = fieldMap.get(fieldId);
if (existingField == null) {
fieldMap.put(fieldId, field);
typeMap.put(fieldId, structField.type());
nameMap.put(fieldId, structField.name());
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
// use the correct type for dropped partitions in v1 tables
if (isVoidTransform(existingField) && !isVoidTransform(field)) {
fieldMap.put(fieldId, field);
typeMap.put(fieldId, structField.type());
}
}
}
}
List<NestedField> sortedStructFields = fieldMap.keySet().stream()
.sorted(Comparator.naturalOrder())
.map(fieldId -> NestedField.optional(fieldId, nameMap.get(fieldId), typeMap.get(fieldId)))
.collect(Collectors.toList());
return StructType.of(sortedStructFields);
```
```
private static boolean isVoidTransform(PartitionField field) {
return field.transform().equals(Transforms.alwaysNull());
}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1049150007
OK I think I understand the issue now, this is specifically an error where the attempt to build a common Partition Spec builds a spec using void transforms in V1 tables, the void column has a different return type than the original partition transform and throws an error. Is that correct?
I would recommend renaming this issue/pr to match the underlying issue. I also am guessing because this is the issue, there is no problem with V2 tables.
@aokolnychyi Should probably also take a look
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] szlta commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
szlta commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r821845093
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I have run into similar issues, and I think this will help resolve the type change of columns in older specs.
Another thing I have seen and is probably still a problem, is that this method may return a column name multiple times. Consider the following:
Table schema: a int, b date, c date
spec0: year(b), a
spec1: a
spec2: year(b), year(c)
then the result is something like this: 1000: b_year int, 1001: a int, 1002: b_year int, 1003: c_year int
Further down the line when we construct a Schema object, we will have a failure due to b_year name being present in two fields (1000, 1002).
How should this case be handled? Maybe appending _r [fieldId] to each column name?
cc: @RussellSpitzer , @aokolnychyi
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r823186537
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I think we have two cases to solve this problem. The combined partition struct type is used in both metadata tables and when we need to project the _partition field for certain queries. For queries that use the _partition field, I think adding a suffix to make the names unique is the right approach. That's internal so it doesn't really matter what we rename to, as long as we get the projections that produce the partition tuple for a given spec right.
For metadata tables, we expect the field names to match the partition names. A simple example is that we use identity partitions using the original column name. So it would be weird to partition by `category` and need to query `category_r1003` in the metadata table. I think that the right thing to do for metadata tables is to have a separate way to produce the combined struct that produces only one partition column per name.
In @aokolnychyi's example, `1001: data` should be present and `1000: data` should not be used for metadata tables.
This is also an area where we may want to bring back compatible partition columns. There's no reason why Anton's example couldn't detect that `1000: data` was in an old spec and reuse the ID to avoid this problem.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1063488197
@ConeyLiu, can you update the description? It is really helpful to not only say what you're trying to fix, but also to describe the approach and why the changes are needed. Thanks!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r823151850
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I guess you are right, @szlta. Here is an example to reproduce.
```
PartitionSpec initialSpec = PartitionSpec.builderFor(SCHEMA)
.identity("data")
.build();
TestTables.TestTable table = TestTables.create(tableDir, "test", SCHEMA, initialSpec, V2_FORMAT_VERSION);
table.updateSpec()
.removeField("data")
.commit();
table.updateSpec()
.addField("data")
.addField("id")
.commit();
struct<1000: data: optional string, 1001: data: optional string, 1002: id: optional int>
```
While it would be great to update the logic that evolves the spec, I think we have to adapt the method that builds a common representation too. Otherwise, existing tables may be broken. Maintaining a set of used names and appending a suffix of the field ID sounds like a reasonable approach.
Thoughts, @rdblue @RussellSpitzer @szehon-ho @flyrain?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825484751
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -222,27 +224,41 @@ public static StructType partitionType(Table table) {
for (PartitionField field : spec.fields()) {
int fieldId = field.fieldId();
+ NestedField structField = spec.partitionType().field(fieldId);
+
PartitionField existingField = fieldMap.get(fieldId);
if (existingField == null) {
fieldMap.put(fieldId, field);
- NestedField structField = spec.partitionType().field(fieldId);
- structFields.add(structField);
+ typeMap.put(fieldId, structField.type());
+ nameMap.put(fieldId, structField.name());
+
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
+
+ // use the correct type for dropped partitions in v1 tables
+ if (isVoidTransform(existingField) && !isVoidTransform(field)) {
+ fieldMap.put(fieldId, field);
+ typeMap.put(fieldId, structField.type());
+ }
}
}
}
- List<NestedField> sortedStructFields = structFields.stream()
- .sorted(Comparator.comparingInt(NestedField::fieldId))
+ List<NestedField> sortedStructFields = fieldMap.keySet().stream()
+ .sorted(Comparator.naturalOrder())
+ .map(fieldId -> NestedField.optional(fieldId, nameMap.get(fieldId), typeMap.get(fieldId)))
Review comment:
I think the changes in this file are correct and fix the first issue, where the type may be incorrect for void transform fields in v1 tables.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r738892510
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
Review comment:
For v1 format, the dropped partition will be replaced with void transform. For example, `bucket(2, string_column)` will be replaced with `void`. However, the type of bucket transform is int, while the type of the void transform is the original field type. So, we get failed.
The common type of dropped partition and the void transform should be the first. So, we replace the void transform with the original transform.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r813255357
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
Review comment:
Do we need the code changes in the other files then? Seems like the only bug is here for V1 tables with columns transformed from a functional partition spec to a void spec with a different type?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1049157138
I may be able to take a look within the next few days
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer merged pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
RussellSpitzer merged pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r821243411
##########
File path: core/src/test/java/org/apache/iceberg/TestPartitioning.java
##########
@@ -140,9 +140,9 @@ public void testPartitionTypeWithAddingBackSamePartitionFieldInV1Table() {
.addField("data")
.commit();
- // in v1, we use void transforms instead of dropping partition fields
+ // in v1, we use void transforms instead of dropping partition fields.
StructType expectedType = StructType.of(
- NestedField.optional(1000, "data_1000", Types.StringType.get()),
+ NestedField.optional(1000, "data", Types.StringType.get()),
Review comment:
This change is wrong. The previous expected value was correct as we need to inherit the newest column names.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#issuecomment-1081319615
Thanks all!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r823185064
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I think Field_id Suffix seems very reasonable to me. Let's be sure your new tests above as well
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825015014
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I'll catch up today. Sorry for the delay!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r825485171
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -210,7 +211,8 @@ public static StructType partitionType(Table table) {
}
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
- List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Type> typeMap = Maps.newHashMap();
+ Map<Integer, String> nameMap = Maps.newHashMap();
Review comment:
I think that the fixes in this PR look good other than a couple minor updates that are needed. We should definitely follow up with a PR that fixes the names as suggested by @szlta.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] aokolnychyi commented on a change in pull request #3411: Fixes read metadata failed after dropped partition for V1 format
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r833614832
##########
File path: core/src/main/java/org/apache/iceberg/ManifestsTable.java
##########
@@ -73,10 +72,15 @@ MetadataTableType metadataTableType() {
protected DataTask task(TableScan scan) {
TableOperations ops = operations();
String location = scan.snapshot().manifestListLocation();
+ Map<Integer, PartitionSpec> specs = Maps.newHashMap(table().specs());
+
return StaticDataTask.of(
ops.io().newInputFile(location != null ? location : ops.current().metadataFileLocation()),
schema(), scan.schema(), scan.snapshot().allManifests(),
- manifest -> ManifestsTable.manifestFileToRow(spec, manifest)
+ manifest -> {
+ PartitionSpec partitionSpec = specs.get(manifest.partitionSpecId());
Review comment:
nit: just `spec`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] RussellSpitzer commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
RussellSpitzer commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r815214752
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -211,6 +211,7 @@ public static StructType partitionType(Table table) {
Map<Integer, PartitionField> fieldMap = Maps.newHashMap();
List<NestedField> structFields = Lists.newArrayList();
+ Map<Integer, Integer> fieldToIndex = Maps.newHashMap();
Review comment:
I still think this needs a new name ... since we only use it to track indexes of void transformed fields
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] ConeyLiu commented on a change in pull request #3411: Fixes read metadata failed after dropped partition
Posted by GitBox <gi...@apache.org>.
ConeyLiu commented on a change in pull request #3411:
URL: https://github.com/apache/iceberg/pull/3411#discussion_r814612980
##########
File path: core/src/main/java/org/apache/iceberg/Partitioning.java
##########
@@ -228,11 +229,22 @@ public static StructType partitionType(Table table) {
fieldMap.put(fieldId, field);
NestedField structField = spec.partitionType().field(fieldId);
structFields.add(structField);
+
+ if (Transforms.alwaysNull().equals(field.transform())) {
+ fieldToIndex.put(fieldId, structFields.size() - 1);
+ }
} else {
// verify the fields are compatible as they may conflict in v1 tables
ValidationException.check(equivalentIgnoringNames(field, existingField),
"Conflicting partition fields: ['%s', '%s']",
field, existingField);
+
+ if (Transforms.alwaysNull().equals(existingField.transform()) &&
+ !Transforms.alwaysNull().equals(field.transform())) {
+ fieldMap.put(fieldId, field);
+ NestedField structField = spec.partitionType().field(fieldId);
+ structFields.set(fieldToIndex.get(fieldId), structField);
Review comment:
```java
fieldMap.put(fieldId, field);
NestedField previousTransform = spec.partitionType().field(fieldId);
int voidTransformIndex = structFields.indexOf(existingField); // here, we need a `NestedField` instance not a `PartitionField` instance, and it seems that we could not get the previous `PartitionField` because we are in new spec.
structFields.set(voidTransformIndex, previousTransform)```
Please see the above inline comments.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org