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 2020/03/16 08:11:46 UTC
[GitHub] [incubator-iceberg] jun-he opened a new pull request #845: Add
persistent IDs to partition fields
jun-he opened a new pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845
For issue https://github.com/apache/incubator-iceberg/issues/281.
I took a different approach from the previous WIP PR (https://github.com/apache/incubator-iceberg/pull/499).
Instead of adding additional states and logics into `TableMetadata`, I created `PartitionSpecUpdate` to handle that. It also does not change `TableMetadataParser` and affects TableMetadata JSON.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406885239
##########
File path: api/src/main/java/org/apache/iceberg/UpdatePartitionSpec.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import org.apache.iceberg.exceptions.CommitFailedException;
+
+/**
+ * API for partition spec evolution.
+ * <p>
+ * When committing, these changes will be applied to the current table metadata. Commit conflicts
+ * will not be resolved and will result in a {@link CommitFailedException}.
+ */
+public interface UpdatePartitionSpec extends PendingUpdate<PartitionSpec> {
Review comment:
For tests, you can use `TableMetadata` and `commit` directly:
```java
TableOperations ops = table.operations();
TableMetadata base = ops.current()
TableMetadata updated = base.updatePartitionSpec(newSpec);
ops.commit(base, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r405298928
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -395,6 +396,47 @@ public void testChangedPartitionSpec() {
initialManifest, pending.manifests().get(1));
}
+ @Test
+ public void testUpdatePartitionSpecFieldIds() {
+ TableMetadata base = readMetadata();
+
+ // build the new spec using the table's schema, which uses fresh IDs
+ PartitionSpec newSpec = PartitionSpec.builderFor(base.schema())
Review comment:
Done. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403188379
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -58,14 +58,16 @@
private transient volatile ListMultimap<Integer, PartitionField> fieldsBySourceId = null;
private transient volatile Class<?>[] lazyJavaClasses = null;
private transient volatile List<PartitionField> fieldList = null;
+ private final int lastAssignedFieldId;
Review comment:
This is tracking something different. Here, this is the highest ID assigned to any partition, so that the next ID assigned will be unique.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406885812
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecParser.java
##########
@@ -138,6 +140,7 @@ private static void buildFromJsonFields(PartitionSpec.Builder builder, JsonNode
"Cannot parse partition spec fields, not an array: %s", json);
Iterator<JsonNode> elements = json.elements();
+ int fieldIdCnt = 0;
Review comment:
Nit: we like to avoid abbreviations that aren't readable. Saving a few chars isn't worth trading readability.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull
request #845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r402947754
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecParser.java
##########
@@ -147,7 +156,12 @@ private static void buildFromJsonFields(PartitionSpec.Builder builder, JsonNode
String transform = JsonUtil.getString(TRANSFORM, element);
int sourceId = JsonUtil.getInt(SOURCE_ID, element);
- builder.add(sourceId, name, transform);
+ // partition field ids are missing in old PartitionSpec, they always auto-increment from PARTITION_DATA_ID_START
+ if (!hasFieldId) {
Review comment:
How about the forward compatibility? Is it possible that an old reader reads the new spec? then it still parses the new spec field id start from 1000?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r408580753
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -487,8 +491,9 @@ public TableMetadata removeSnapshotLogEntries(Set<Long> snapshotIds) {
currentSnapshotId, snapshots, newSnapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec updatedPartitionSpec,
- Map<String, String> updatedProperties) {
+ // updatedPartitionSpec's partition field IDs should have already been refreshed
+ TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec updatedPartitionSpec,
Review comment:
@rdblue thanks for the comment.
I am thinking to expose those functionalities (`PartitionSpecUpdate` or `buildReplacement`) over `Table` and `UpdatePartitionSpec` public interface. So we can limit their access to be package/default level.
I will change them back to public in the PR and we may limit their access in the partition spec evolution PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r405304258
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -307,11 +312,16 @@ public static Builder builderFor(Schema schema) {
private final Set<String> partitionNames = Sets.newHashSet();
private Map<Integer, PartitionField> timeFields = Maps.newHashMap();
private int specId = 0;
+ private int lastAssignedFieldId = PARTITION_DATA_ID_START - 1;
Review comment:
I think we still have to use `PARTITION_DATA_ID_START - 1` to handle the case that there is no assigned field Id. Or we can update this field as `nextFieldId` and then set it from `PARTITION_DATA_ID_START`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406889474
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -487,8 +491,9 @@ public TableMetadata removeSnapshotLogEntries(Set<Long> snapshotIds) {
currentSnapshotId, snapshots, newSnapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec updatedPartitionSpec,
- Map<String, String> updatedProperties) {
+ // updatedPartitionSpec's partition field IDs should have already been refreshed
+ TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec updatedPartitionSpec,
+ Map<String, String> updatedProperties) {
Review comment:
We should add a validation for format v1 here 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406890457
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -350,7 +354,8 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
+ // newPartitionSpec's partition field IDs should have already been refreshed
+ TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
Review comment:
You're right. We just need to check that ids are sequential and starting at 1,000 if the table version is v1.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403267125
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -350,7 +354,8 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
+ // newPartitionSpec's partition field IDs should have already been refreshed
Review comment:
Spec field IDs don't need to be refreshed. `freshSpec` is used for two things:
1. If schema IDs are reassigned, then it rebuilds the spec to with the new schema
2. It sets the spec's ID to the one assigned in this method
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403232756
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecParser.java
##########
@@ -138,6 +140,13 @@ private static void buildFromJsonFields(PartitionSpec.Builder builder, JsonNode
"Cannot parse partition spec fields, not an array: %s", json);
Iterator<JsonNode> elements = json.elements();
+
+ boolean hasFieldId = false;
+ if (elements.hasNext() && elements.next().has(FIELD_ID)) {
+ hasFieldId = true;
+ }
+ elements = json.elements();
Review comment:
I think it would be easier to follow and would result in a better error message if we put this logic inside the elements loop.
How about adding a counter for field IDs that are present and after the loop throwing an exception if the counter is not equal to the number of fields? Then each field would be handled independently (using `has(FIELD_ID)`).
I like the idea of a check here that states there were missing field IDs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull
request #845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r402925293
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -395,6 +396,47 @@ public void testChangedPartitionSpec() {
initialManifest, pending.manifests().get(1));
}
+ @Test
+ public void testUpdatePartitionSpecFieldIds() {
+ TableMetadata base = readMetadata();
+
+ // build the new spec using the table's schema, which uses fresh IDs
+ PartitionSpec newSpec = PartitionSpec.builderFor(base.schema())
Review comment:
Can we add a unit test that has two manifests with the different specs of the same source field, and then check whether the partition field ids of manifest entries are different?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406629266
##########
File path: api/src/main/java/org/apache/iceberg/UpdatePartitionSpec.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import org.apache.iceberg.exceptions.CommitFailedException;
+
+/**
+ * API for partition spec evolution.
+ * <p>
+ * When committing, these changes will be applied to the current table metadata. Commit conflicts
+ * will not be resolved and will result in a {@link CommitFailedException}.
+ */
+public interface UpdatePartitionSpec extends PendingUpdate<PartitionSpec> {
Review comment:
I agree that it is better to move the update to a new PR, which addresses #281.
The main reason I put it here is to have additional unit tests to make sure it works as expected.
Additionally, I think we may support the table partition spec evolution in two ways
```
table.updatePartitionSpec()
.update(spec)
.commit();
```
```
table.updatePartitionSpec().newSpec(schema)
.identity(...)
.bucket(...)
...
.commit();
```
The first approach may be used if clients want to define a spec and manage it by their codes, e.g. use defined spec object in multiple places.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #845: Add persistent
IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#issuecomment-608612302
I'm only about half-way done reviewing this, but I wanted to capture some thoughts about forward-compatibility that was raised by @chenjunjiedada.
If there are already multiple partition specs, then the IDs may be reused and can even conflict. This isn't something we can change because manifest files embed the field IDs in their schemas. That means assignment when there are no IDs must be from 1000 and should be independent across different partition specs.
If an older version writes to the table, then it may remove any assigned partition IDs. That means that for any format v1 table, we must remain compatible with the current assignment strategy. That way, IDs can be removed by an old writer and will be the same when they are reassigned.
This also means that evolution is limited in v1 tables. To ensure that IDs can be reassigned correctly if they are removed, partition fields cannot be dropped or reordered in any way. Otherwise, reassignment would be incorrect. That means no removing partition fields, no reordering partition fields, and no adding partition fields unless they are added at the end of the spec.
We will be able to make more evolution changes when we can guarantee that all partition fields have IDs that won't be removed. We'll make the IDs a requirement in v2 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406887795
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -487,8 +491,9 @@ public TableMetadata removeSnapshotLogEntries(Set<Long> snapshotIds) {
currentSnapshotId, snapshots, newSnapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec updatedPartitionSpec,
- Map<String, String> updatedProperties) {
+ // updatedPartitionSpec's partition field IDs should have already been refreshed
+ TableMetadata buildReplacement(Schema updatedSchema, PartitionSpec updatedPartitionSpec,
Review comment:
Why is this no longer public?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull
request #845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r402895252
##########
File path: api/src/main/java/org/apache/iceberg/PartitionField.java
##########
@@ -28,11 +28,13 @@
*/
public class PartitionField implements Serializable {
private final int sourceId;
+ private final int fieldId;
private final String name;
private final Transform<?, ?> transform;
- PartitionField(int sourceId, String name, Transform<?, ?> transform) {
+ PartitionField(int sourceId, int fieldId, String name, Transform<?, ?> transform) {
this.sourceId = sourceId;
+ this.fieldId = fieldId;
Review comment:
Do we need to check `fieldId` is larger than 1000?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r405309135
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -350,7 +354,8 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
+ // newPartitionSpec's partition field IDs should have already been refreshed
Review comment:
This comment is about the input parameter `PartitionSpec newPartitionSpec`, whose field ids has to be processed/refreshed so that `freshSpec` does not need to take care of that extra work.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r408618872
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -373,10 +374,10 @@ public void testChangedPartitionSpec() {
.bucket("id", 4)
.build();
- // commit the new partition spec to the table manually
- table.ops().commit(base, base.updatePartitionSpec(newSpec));
+ // commit the new partition spec to the table
+ table.updatePartitionSpec().update(newSpec).commit();
Review comment:
👌
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403270004
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecParser.java
##########
@@ -147,7 +156,12 @@ private static void buildFromJsonFields(PartitionSpec.Builder builder, JsonNode
String transform = JsonUtil.getString(TRANSFORM, element);
int sourceId = JsonUtil.getInt(SOURCE_ID, element);
- builder.add(sourceId, name, transform);
+ // partition field ids are missing in old PartitionSpec, they always auto-increment from PARTITION_DATA_ID_START
+ if (!hasFieldId) {
Review comment:
Instead of making these changes here, I think that this should be verified in `TableMetadata`. That would accomplish the same thing, but make it easier to check the table version.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403269235
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -350,7 +354,8 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
+ // newPartitionSpec's partition field IDs should have already been refreshed
+ TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
Review comment:
These should still be public like the other methods. Instead of making them private, they should sanity check the spec:
* For a v1 table, field IDs should be assigned from 1000 and should have no gaps
* For a v2 table or newer, all fields should have IDs
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403221964
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -437,24 +447,30 @@ public Builder truncate(String sourceName, int width, String targetName) {
checkAndAddPartitionName(targetName);
Types.NestedField sourceColumn = findSourceColumn(sourceName);
fields.add(new PartitionField(
- sourceColumn.fieldId(), targetName, Transforms.truncate(sourceColumn.type(), width)));
+ sourceColumn.fieldId(), nextFieldId(), targetName, Transforms.truncate(sourceColumn.type(), width)));
return this;
}
public Builder truncate(String sourceName, int width) {
return truncate(sourceName, width, sourceName + "_trunc");
}
- Builder add(int sourceId, String name, String transform) {
+ // add a partition field with an auto-increment partition field id starting from PARTITION_DATA_ID_START
+ Builder append(int sourceId, String name, String transform) {
Review comment:
Why rename this to `append`? What about `addWithoutId` or just overload `add`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r405304823
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -437,24 +447,30 @@ public Builder truncate(String sourceName, int width, String targetName) {
checkAndAddPartitionName(targetName);
Types.NestedField sourceColumn = findSourceColumn(sourceName);
fields.add(new PartitionField(
- sourceColumn.fieldId(), targetName, Transforms.truncate(sourceColumn.type(), width)));
+ sourceColumn.fieldId(), nextFieldId(), targetName, Transforms.truncate(sourceColumn.type(), width)));
return this;
}
public Builder truncate(String sourceName, int width) {
return truncate(sourceName, width, sourceName + "_trunc");
}
- Builder add(int sourceId, String name, String transform) {
+ // add a partition field with an auto-increment partition field id starting from PARTITION_DATA_ID_START
+ Builder append(int sourceId, String name, String transform) {
Review comment:
Sure, change it back to `add`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406646272
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -350,7 +354,8 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
+ // newPartitionSpec's partition field IDs should have already been refreshed
+ TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
Review comment:
As this check and field ID change will be in the same PR, do we still need to check if all fields should have IDs in `newPartitionSpec`? When the check is called, the PartitionSpec must have a field ID, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406632269
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecParser.java
##########
@@ -138,6 +140,13 @@ private static void buildFromJsonFields(PartitionSpec.Builder builder, JsonNode
"Cannot parse partition spec fields, not an array: %s", json);
Iterator<JsonNode> elements = json.elements();
+
+ boolean hasFieldId = false;
+ if (elements.hasNext() && elements.next().has(FIELD_ID)) {
+ hasFieldId = true;
+ }
+ elements = json.elements();
Review comment:
👌
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on issue #845: Add persistent
IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on issue #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#issuecomment-602377013
@rdblue can you please take a look? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #845: Add persistent
IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#issuecomment-608625482
Thanks @jun-he, this is great progress on partition field IDs! I think I understand how compatibility will work with field IDs and I tried to add that context to my comments in this PR. If it isn't clear, please let me know what doesn't make sense.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403262319
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -204,8 +205,11 @@ public String toString() {
this.lastUpdatedMillis = lastUpdatedMillis;
this.lastColumnId = lastColumnId;
this.schema = schema;
- this.specs = specs;
this.defaultSpecId = defaultSpecId;
+
+ Preconditions.checkArgument(specs != null && !specs.isEmpty(), "specs cannot be null or empty");
+ this.specs = specs;
Review comment:
We typically validate arguments before assignments, to keep the code easier to read and maintain. Can you move the precondition above this section? Also, error messages should use sentence case so capitalize `Specs`. If you're referring to a variable name, then consider describing what it is instead (Partition specs instead of `specs`).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] chenjunjiedada commented on a change in pull
request #845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
chenjunjiedada commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r402890960
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -58,14 +58,16 @@
private transient volatile ListMultimap<Integer, PartitionField> fieldsBySourceId = null;
private transient volatile Class<?>[] lazyJavaClasses = null;
private transient volatile List<PartitionField> fieldList = null;
+ private final int lastAssignedFieldId;
Review comment:
Do we want to use TypeUtil::NextID?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on issue #845: Add persistent
IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on issue #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#issuecomment-611925824
I will move `UpdatePartitionSpec` and all related unit tests to another PR.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403220969
##########
File path: api/src/main/java/org/apache/iceberg/PartitionSpec.java
##########
@@ -307,11 +312,16 @@ public static Builder builderFor(Schema schema) {
private final Set<String> partitionNames = Sets.newHashSet();
private Map<Integer, PartitionField> timeFields = Maps.newHashMap();
private int specId = 0;
+ private int lastAssignedFieldId = PARTITION_DATA_ID_START - 1;
Review comment:
We tend to prefer using `AtomicInteger` instead of an `int` field. That way it is more clear because the calls are `incrementAndGet` or `getAndIncrement`. That would also avoid the need to start at `PARTITION_DATA_ID_START - 1`. You could use the constant to initialize and assign using `getAndIncrement`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403186492
##########
File path: api/src/main/java/org/apache/iceberg/PartitionField.java
##########
@@ -28,11 +28,13 @@
*/
public class PartitionField implements Serializable {
private final int sourceId;
+ private final int fieldId;
private final String name;
private final Transform<?, ?> transform;
- PartitionField(int sourceId, String name, Transform<?, ?> transform) {
+ PartitionField(int sourceId, int fieldId, String name, Transform<?, ?> transform) {
this.sourceId = sourceId;
+ this.fieldId = fieldId;
Review comment:
I don't think so. That is a convention that we use, but not strictly required by the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r408601540
##########
File path: api/src/main/java/org/apache/iceberg/UpdatePartitionSpec.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import org.apache.iceberg.exceptions.CommitFailedException;
+
+/**
+ * API for partition spec evolution.
+ * <p>
+ * When committing, these changes will be applied to the current table metadata. Commit conflicts
+ * will not be resolved and will result in a {@link CommitFailedException}.
+ */
+public interface UpdatePartitionSpec extends PendingUpdate<PartitionSpec> {
Review comment:
👌
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406886278
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecParser.java
##########
@@ -147,7 +150,17 @@ private static void buildFromJsonFields(PartitionSpec.Builder builder, JsonNode
String transform = JsonUtil.getString(TRANSFORM, element);
int sourceId = JsonUtil.getInt(SOURCE_ID, element);
- builder.add(sourceId, name, transform);
+ // partition field ids are missing in old PartitionSpec, they always auto-increment from PARTITION_DATA_ID_START
+ if (element.has(FIELD_ID)) {
+ builder.add(sourceId, JsonUtil.getInt(FIELD_ID, element), name, transform);
+ fieldIdCnt++;
+ } else {
+ builder.add(sourceId, name, transform);
+ }
}
+
+ Preconditions.checkArgument(fieldIdCnt == 0 || fieldIdCnt == json.size(),
+ "Parsed an invalid PartitionSpec Json. Some of its fields (%s/%s) miss field IDs.",
Review comment:
How about "Cannot parse spec with missing field IDs: %s missing of %s 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406620284
##########
File path: api/src/main/java/org/apache/iceberg/PartitionField.java
##########
@@ -60,7 +69,7 @@ public String name() {
@Override
public String toString() {
- return name + ": " + transform + "(" + sourceId + ")";
+ return name + " (" + fieldId + "): " + transform + "(" + sourceId + ")";
Review comment:
👌
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r409209633
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -648,4 +649,98 @@ public void testInvalidAppendManifest() throws IOException {
.appendManifest(manifestWithDeletedFiles)
.commit());
}
+
Review comment:
Thanks. Will remove it in https://github.com/apache/incubator-iceberg/pull/922.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on issue #845: Add persistent
IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on issue #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#issuecomment-605700071
Hi @rdblue, can you please take a look? 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403257916
##########
File path: core/src/main/java/org/apache/iceberg/PartitionSpecParser.java
##########
@@ -147,7 +156,12 @@ private static void buildFromJsonFields(PartitionSpec.Builder builder, JsonNode
String transform = JsonUtil.getString(TRANSFORM, element);
int sourceId = JsonUtil.getInt(SOURCE_ID, element);
- builder.add(sourceId, name, transform);
+ // partition field ids are missing in old PartitionSpec, they always auto-increment from PARTITION_DATA_ID_START
+ if (!hasFieldId) {
Review comment:
For forward-compatibility, I think that this should detect breaking changes to specs and throw an exception.
If IDs are removed by an older writer, then the IDs will be reassigned. That means that IDs must be assigned starting at 1000 and should have no gaps. If there are IDs, this should validate that assumption by checking that the field actually has the ID that is expected.
We should make a similar change on the write path: for each field, check that it's field ID is what would be assigned if it were removed by an older writer. That will prevent newer writers from creating specs that will break.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406887445
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -208,8 +210,9 @@ public String toString() {
this.lastUpdatedMillis = lastUpdatedMillis;
this.lastColumnId = lastColumnId;
this.schema = schema;
- this.specs = specs;
this.defaultSpecId = defaultSpecId;
+ this.specs = specs;
+
Review comment:
Can you revert these unnecessary changes? The order of assignments changed and there's an extra newline. These non-functional changes can cause git conflicts, so we try to avoid them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406884244
##########
File path: api/src/main/java/org/apache/iceberg/Transaction.java
##########
@@ -40,6 +40,14 @@
*/
UpdateSchema updateSchema();
+
Review comment:
Nit: unnecessary newline.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r405309135
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -350,7 +354,8 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
- public TableMetadata updatePartitionSpec(PartitionSpec newPartitionSpec) {
+ // newPartitionSpec's partition field IDs should have already been refreshed
Review comment:
This comment is about the input parameter `PartitionSpec newPartitionSpec`, whose field ids has to be recomputed so that `freshSpec` does not need to take care of that extra work.
Updated this comment to make it clear.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406889223
##########
File path: core/src/main/java/org/apache/iceberg/TableMetadata.java
##########
@@ -352,6 +355,7 @@ public TableMetadata updateSchema(Schema newSchema, int newLastColumnId) {
currentSnapshotId, snapshots, snapshotLog, addPreviousFile(file, lastUpdatedMillis));
}
+ // Input newPartitionSpec's partition field IDs should have already been recomputed
Review comment:
#903 added `formatVersion` to metadata, so now we can add a validation here when the table is v1 that the spec IDs are assigned as expected:
```
ValidationException.check(formatVersion > 1 || hasSequentialIds(spec),
"Spec does not use sequential IDs that are required in v1: %s", 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] jun-he commented on issue #845: Add persistent
IDs to partition fields
Posted by GitBox <gi...@apache.org>.
jun-he commented on issue #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#issuecomment-614194662
@chenjunjiedada FYI, I refactored the code and the partition spec evolution changes and related tests are now in https://github.com/apache/incubator-iceberg/pull/922. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403275025
##########
File path: api/src/main/java/org/apache/iceberg/UpdatePartitionSpec.java
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.iceberg;
+
+import org.apache.iceberg.exceptions.CommitFailedException;
+
+/**
+ * API for partition spec evolution.
+ * <p>
+ * When committing, these changes will be applied to the current table metadata. Commit conflicts
+ * will not be resolved and will result in a {@link CommitFailedException}.
+ */
+public interface UpdatePartitionSpec extends PendingUpdate<PartitionSpec> {
Review comment:
Can we remove the new interface and methods from this PR?
I don't think this is needed for this PR, and I'd like to minimize the number of changes. In addition, I don't think we want to move to a model where users create a new spec and apply it to a table. I think we instead want to evolve the partition spec of a table. So this API will probably be different when we release that feature.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on issue #845: Add persistent
IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on issue #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#issuecomment-614256946
Awesome work, @jun-he! I'm merging 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r409102741
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -648,4 +649,98 @@ public void testInvalidAppendManifest() throws IOException {
.appendManifest(manifestWithDeletedFiles)
.commit());
}
+
Review comment:
Nit: unnecessary newline.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue merged pull request #845: Add persistent
IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r403187322
##########
File path: api/src/main/java/org/apache/iceberg/PartitionField.java
##########
@@ -60,7 +69,7 @@ public String name() {
@Override
public String toString() {
- return name + ": " + transform + "(" + sourceId + ")";
+ return name + " (" + fieldId + "): " + transform + "(" + sourceId + ")";
Review comment:
Nit: schemas use the format `1000: ts_day`. That would be a bit cleaner than two sets of parens.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [incubator-iceberg] rdblue commented on a change in pull request
#845: Add persistent IDs to partition fields
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #845: Add persistent IDs to partition fields
URL: https://github.com/apache/incubator-iceberg/pull/845#discussion_r406889814
##########
File path: core/src/test/java/org/apache/iceberg/TestMergeAppend.java
##########
@@ -373,10 +374,10 @@ public void testChangedPartitionSpec() {
.bucket("id", 4)
.build();
- // commit the new partition spec to the table manually
- table.ops().commit(base, base.updatePartitionSpec(newSpec));
+ // commit the new partition spec to the table
+ table.updatePartitionSpec().update(newSpec).commit();
Review comment:
As I mentioned earlier, I think this should continue using the `updatePartitionSpec` method so that we can add a public API for this in a separate commit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org