You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "shanielh (via GitHub)" <gi...@apache.org> on 2024/07/21 11:47:42 UTC
[PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
shanielh opened a new pull request, #10737:
URL: https://github.com/apache/iceberg/pull/10737
Most of the query engines use SQL to update a partition specification of a table.
The SQL usually look like: alter table partition by a, b, c, ...
The interface of UpdatePartitionSpec is interrupting a clear flow of setting a new partition spec when the user specifies a full partition definition, because it requires the implementation of the command to remove / rename old fields and add new fields that didn't exist.
Instead of that, we can introduce a new command to Iceberg Tables that adds a new partition spec from scratch.
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1698599115
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -66,20 +66,36 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ fromSpec(base.spec());
+ }
+
+ @Override
+ public UpdatePartitionSpec fromSpec(PartitionSpec partitionSpec) {
+ // Clear all changes
Review Comment:
> Calling Add Identity(X), and then Remove Identity(X) within the same UpdatePartitionSpec throws an exception:
>
> ```
> Cannot delete newly added field: 1001: id: identity(1)
> java.lang.IllegalArgumentException: Cannot delete newly added field: 1001: id: identity(1)
> at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:220)
> at org.apache.iceberg.BaseUpdatePartitionSpec.removeField(BaseUpdatePartitionSpec.java:247)
> ```
Not in the same command.
Two commands
Command 1
Add Identity(id)
commit()
Command 2
From unpartitioned
Add Identity(id)
commit() // This should fail I think? or be a noop?
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1688720424
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
Instead of adding a fully new API couldn't we just add an API to UpdatePartitionSpec that just sets the starting point of the spec as unpartitioned?
Something like
```
updatePartitionSpec()
.fromUnpartitioned()
.add...
.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.
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703493428
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -66,20 +66,36 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ fromSpec(base.spec());
+ }
+
+ @Override
+ public UpdatePartitionSpec fromSpec(PartitionSpec partitionSpec) {
+ // Clear all changes
Review Comment:
@RussellSpitzer any chance to look at this? I've added the specs you wanted in the last iteration. 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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1694103029
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
~Ok I think it should pass now, I've an ignore to the revapi (after rebasing on apache/iceberg), I hope that what I should have done π ~
Moving the new method in the interface to the bottom worked without modifying revapi
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
~Ok I think it should pass now, I've an ignore to the revapi (after rebasing on apache/iceberg), I hope that what I should have done π~
Moving the new method in the interface to the bottom worked without modifying revapi
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1688993988
##########
api/src/main/java/org/apache/iceberg/Transaction.java:
##########
@@ -44,6 +44,13 @@ public interface Transaction {
*/
UpdatePartitionSpec updateSpec();
+ /**
+ * Create a new {@link AddPartitionSpec} to alter the partition spec of this table.
Review Comment:
Correct
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1689195839
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
I reverted the old commit and added a new one with an implementation of `fromSpec(..)`, and also added a test. PLMK if there's anything else I need to do for this to get merged π
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1688711598
##########
api/src/main/java/org/apache/iceberg/Table.java:
##########
@@ -183,6 +183,14 @@ default IncrementalChangelogScan newIncrementalChangelogScan() {
*/
UpdatePartitionSpec updateSpec();
+ /**
+ * Create a new {@link AddPartitionSpec} to alter the partition spec of this table and commit the
Review Comment:
Same comment as below on Java Doc
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1694103029
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
Ok I think it should pass now, I've an ignore to the revapi, I hope that what I should have done π
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
Ok I think it should pass now, I've an ignore to the revapi (after rebasing on apache/iceberg), I hope that what I should have done π
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#issuecomment-2466519723
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think thatβs incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703505303
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -90,6 +90,46 @@ public void testCommitUpdatedSpec() {
assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1002);
}
+ @TestTemplate
+ public void testCommitFromSpec() {
+ table.updateSpec().addField(bucket("id", 8)).commit();
+
+ // Evolve the spec
Review Comment:
I would avoid generic comments like this if possible
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703554438
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -62,15 +62,20 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
private int lastAssignedPartitionId;
BaseUpdatePartitionSpec(TableOperations ops) {
+ this(ops, ops.current().spec());
+ }
+
+ BaseUpdatePartitionSpec(TableOperations ops, PartitionSpec spec) {
this.ops = ops;
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ this.spec = spec;
this.schema = spec.schema();
this.nameToField = indexSpecByName(spec);
this.transformToField = indexSpecByTransform(spec);
- this.lastAssignedPartitionId = base.lastAssignedPartitionId();
+ this.lastAssignedPartitionId =
Review Comment:
As the spec requires:
```
In v2, partition field IDs must be explicitly tracked for each partition field. New IDs are assigned based on the last assigned partition ID in table metadata.
In v1, partition field IDs were not tracked, but were assigned sequentially starting at 1000 in the reference implementation.
```
Since we always evolve the latest spec, `base.lastAssignedPartitionId` would work for both v1 and v2, but now when we're able to evolve from any spec (which might be non-latest), it doesn't work and requires a branch in the code.
Reverting this would fail some of the new tests:
- `testReAddFieldUsingFromUnpartitionedSpec`
- π΄ `formatVersion = 1`
- β
`formatVersion = 2`
- β
`formatVersion = 3`
- `testCommitFromSpec`
- π΄ `formatVersion = 1`
- β
`formatVersion = 2`
- β
`formatVersion = 3`
I did modified the branch to condition on `formatVersion == 1` instead of `formatVersion == 2` for better forward compatibility.
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1695794990
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -66,20 +66,36 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ fromSpec(base.spec());
+ }
+
+ @Override
+ public UpdatePartitionSpec fromSpec(PartitionSpec partitionSpec) {
+ // Clear all changes
Review Comment:
I'm also wondering if we may have issues here with re-adding a transform that already exists in another spec. I think currently that's handled by re-using the existing transform.
Example
```
Add Identity (x)
Remove Identity (x)
Add Identity (x)
```
In the current code there would only ever be one Identity(x) transform. Would we be able to maintain that if we did
```
Add Identity(x)
From(Unpartitioned)
Add Identity(x)
```
The above code should be a noop 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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703630708
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -62,15 +62,20 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
private int lastAssignedPartitionId;
BaseUpdatePartitionSpec(TableOperations ops) {
+ this(ops, ops.current().spec());
+ }
+
+ BaseUpdatePartitionSpec(TableOperations ops, PartitionSpec spec) {
this.ops = ops;
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ this.spec = spec;
this.schema = spec.schema();
this.nameToField = indexSpecByName(spec);
this.transformToField = indexSpecByTransform(spec);
- this.lastAssignedPartitionId = base.lastAssignedPartitionId();
+ this.lastAssignedPartitionId =
Review Comment:
Ok, got it, I think, can you check if the test is testing things as expected?
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1688708763
##########
api/src/main/java/org/apache/iceberg/Transaction.java:
##########
@@ -44,6 +44,13 @@ public interface Transaction {
*/
UpdatePartitionSpec updateSpec();
+ /**
+ * Create a new {@link AddPartitionSpec} to alter the partition spec of this table.
Review Comment:
This would only be able to add a new Partition Spec to the table correct? We can't actually alter any existing 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.
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1688994691
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
Yeah I think I can do that. Maybe from(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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1698550513
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -66,20 +66,36 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ fromSpec(base.spec());
+ }
+
+ @Override
+ public UpdatePartitionSpec fromSpec(PartitionSpec partitionSpec) {
+ // Clear all changes
Review Comment:
> Implementation wise, I would rather we return a new object here? Couldn't we add a constructor
>
> BaseUpdatePartitionSpec(TableOperations ops, PartitionSpec spec)
>
> That way we could keep all of the variables above final?
Modified and fixed revApi
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1694103029
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
~Ok I think it should pass now, I've an ignore to the revapi (after rebasing on apache/iceberg), I hope that what I should have done π ~
Moving the method in the interface to bottom worked.
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1695790388
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -66,20 +66,36 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ fromSpec(base.spec());
+ }
+
+ @Override
+ public UpdatePartitionSpec fromSpec(PartitionSpec partitionSpec) {
+ // Clear all changes
Review Comment:
Implementation wise, I would rather we return a new object here? Couldn't we add a constructor
BaseUpdatePartitionSpec(TableOperations ops, PartitionSpec spec)
That way we could keep all of the variables above final?
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#issuecomment-2480862579
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "jasonf20 (via GitHub)" <gi...@apache.org>.
jasonf20 commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1698628505
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -66,20 +66,36 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ fromSpec(base.spec());
+ }
+
+ @Override
+ public UpdatePartitionSpec fromSpec(PartitionSpec partitionSpec) {
+ // Clear all changes
Review Comment:
Those seem to be the specs that @shanielh added, resulting in a noop.
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1698579700
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -66,20 +66,36 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ fromSpec(base.spec());
+ }
+
+ @Override
+ public UpdatePartitionSpec fromSpec(PartitionSpec partitionSpec) {
+ // Clear all changes
Review Comment:
> I'm also wondering if we may have issues here with re-adding a transform that already exists in another spec. I think currently that's handled by re-using the existing transform.
>
> Example
>
> ```
> Add Identity (x)
> Remove Identity (x)
> Add Identity (x)
> ```
>
> In the current code there would only ever be one Identity(x) transform. Would we be able to maintain that if we did
>
> ```
> Add Identity(x)
> From(Unpartitioned)
> Add Identity(x)
> ```
>
> The above code should be a noop right?
I've added more specs, I hope that would answer your question.
Calling Add Identity(X), and then Remove Identity(X) within the same UpdatePartitionSpec throws an exception:
```
Cannot delete newly added field: 1001: id: identity(1)
java.lang.IllegalArgumentException: Cannot delete newly added field: 1001: id: identity(1)
at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkArgument(Preconditions.java:220)
at org.apache.iceberg.BaseUpdatePartitionSpec.removeField(BaseUpdatePartitionSpec.java:247)
```
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703502870
##########
api/src/main/java/org/apache/iceberg/UpdatePartitionSpec.java:
##########
@@ -29,6 +29,7 @@
* will not be resolved and will result in a {@link CommitFailedException}.
*/
public interface UpdatePartitionSpec extends PendingUpdate<PartitionSpec> {
+
Review Comment:
unrelated white-space change
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703504713
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -62,15 +62,20 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
private int lastAssignedPartitionId;
BaseUpdatePartitionSpec(TableOperations ops) {
+ this(ops, ops.current().spec());
+ }
+
+ BaseUpdatePartitionSpec(TableOperations ops, PartitionSpec spec) {
this.ops = ops;
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ this.spec = spec;
this.schema = spec.schema();
this.nameToField = indexSpecByName(spec);
this.transformToField = indexSpecByTransform(spec);
- this.lastAssignedPartitionId = base.lastAssignedPartitionId();
+ this.lastAssignedPartitionId =
Review Comment:
I'm not sure I understand the change here, why would lastAssignedPartitionId be drawn from the "spec"? Shouldn't this always be the lastAssignedPartitionId?
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "shanielh (via GitHub)" <gi...@apache.org>.
shanielh commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703549673
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -90,6 +90,46 @@ public void testCommitUpdatedSpec() {
assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1002);
}
+ @TestTemplate
+ public void testCommitFromSpec() {
+ table.updateSpec().addField(bucket("id", 8)).commit();
+
+ // Evolve the spec
+ PartitionSpec evolvedSpec =
+ PartitionSpec.builderFor(table.schema())
+ .withSpecId(1)
+ .bucket("data", 16)
+ .bucket("id", 8, "id_bucket_8")
+ .build();
+ assertThat(table.spec())
+ .as("Should append a partition field to the spec")
+ .isEqualTo(evolvedSpec);
+ assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1001);
+
+ // Restart the spec
+ table
+ .updateSpec()
+ .fromSpec(PartitionSpec.builderFor(table.schema()).build())
Review Comment:
This won't work as `PartitionSpec.unpartitioned()` has an empty schema, and the line after (`.addField(bucket("data", 16))`) would throw: `Cannot find field 'data' in struct: struct<>`
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #10737: AddPartitionSpec: A new way to set new partition specs
URL: https://github.com/apache/iceberg/pull/10737
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1691611196
##########
api/src/main/java/org/apache/iceberg/AddPartitionSpec.java:
##########
@@ -0,0 +1,91 @@
+/*
+ * 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;
+import org.apache.iceberg.expressions.Expressions;
+import org.apache.iceberg.expressions.Term;
+
+/**
+ * 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 AddPartitionSpec extends PendingUpdate<PartitionSpec> {
Review Comment:
Tests are still failing at the moment, please check out the failed runs below
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703559470
##########
core/src/main/java/org/apache/iceberg/BaseUpdatePartitionSpec.java:
##########
@@ -62,15 +62,20 @@ class BaseUpdatePartitionSpec implements UpdatePartitionSpec {
private int lastAssignedPartitionId;
BaseUpdatePartitionSpec(TableOperations ops) {
+ this(ops, ops.current().spec());
+ }
+
+ BaseUpdatePartitionSpec(TableOperations ops, PartitionSpec spec) {
this.ops = ops;
this.caseSensitive = true;
this.base = ops.current();
this.formatVersion = base.formatVersion();
- this.spec = base.spec();
+ this.spec = spec;
this.schema = spec.schema();
this.nameToField = indexSpecByName(spec);
this.transformToField = indexSpecByTransform(spec);
- this.lastAssignedPartitionId = base.lastAssignedPartitionId();
+ this.lastAssignedPartitionId =
Review Comment:
I'm not sure I follow and I don't think this is correct. I think if base.lastMetadataAssigned doesn't work then the logic in the code is incorrect for using this value/ re-using existing fields.
In V1 and V2 it should be the same.
V1:
```
Add a transform identity(x) and get 1000 as the id
Spec 1 = (1000: Identity(x))
Remove identity(x), the transform is changed to void(x) in the new spec.
Spec 2 = (1000: Void())
Add identity(x)
This should reset current spec to Spec 1
Add identity (y)
Spec 3 = (1000: Identity (x), 1001: Identity(y))
```
V2
```
Add a transform identity(x) and get 1000 as the id
Spec 1 = (1000: Identity(x))
Remove identity(x), the transform is changed to void(x) in the new spec.
Spec 2 = ()
Add identity(x)
This should reset current spec to Spec 1
Add identity(y)
Spec 3 = (1000: Identity(x), 1001: identity (y))
```
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703506059
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -90,6 +90,46 @@ public void testCommitUpdatedSpec() {
assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1002);
}
+ @TestTemplate
+ public void testCommitFromSpec() {
+ table.updateSpec().addField(bucket("id", 8)).commit();
+
+ // Evolve the spec
+ PartitionSpec evolvedSpec =
+ PartitionSpec.builderFor(table.schema())
+ .withSpecId(1)
+ .bucket("data", 16)
+ .bucket("id", 8, "id_bucket_8")
+ .build();
+ assertThat(table.spec())
+ .as("Should append a partition field to the spec")
+ .isEqualTo(evolvedSpec);
+ assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1001);
+
+ // Restart the spec
+ table
+ .updateSpec()
+ .fromSpec(PartitionSpec.builderFor(table.schema()).build())
Review Comment:
Shouldn't this just be PartitionSpec.unpartitioned()?
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703507372
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -90,6 +90,46 @@ public void testCommitUpdatedSpec() {
assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1002);
}
+ @TestTemplate
+ public void testCommitFromSpec() {
+ table.updateSpec().addField(bucket("id", 8)).commit();
+
+ // Evolve the spec
+ PartitionSpec evolvedSpec =
+ PartitionSpec.builderFor(table.schema())
+ .withSpecId(1)
+ .bucket("data", 16)
+ .bucket("id", 8, "id_bucket_8")
+ .build();
+ assertThat(table.spec())
+ .as("Should append a partition field to the spec")
+ .isEqualTo(evolvedSpec);
+ assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1001);
+
+ // Restart the spec
+ table
+ .updateSpec()
+ .fromSpec(PartitionSpec.builderFor(table.schema()).build())
+ .addField(bucket("id", 8))
+ .commit();
+
+ V1Assert.assertEquals(
+ "Should soft delete id and data buckets",
+ PartitionSpec.builderFor(table.schema())
+ .withSpecId(2)
+ .bucket("id", 8, "id_bucket_8")
+ .build(),
+ table.spec());
+
+ V2Assert.assertEquals(
+ "Should hard delete id and data buckets",
Review Comment:
Same comment as above, we should not delete the id bucket
--
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
Re: [PR] AddPartitionSpec: A new way to set new partition specs [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #10737:
URL: https://github.com/apache/iceberg/pull/10737#discussion_r1703507104
##########
core/src/test/java/org/apache/iceberg/TestTableUpdatePartitionSpec.java:
##########
@@ -90,6 +90,46 @@ public void testCommitUpdatedSpec() {
assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1002);
}
+ @TestTemplate
+ public void testCommitFromSpec() {
+ table.updateSpec().addField(bucket("id", 8)).commit();
+
+ // Evolve the spec
+ PartitionSpec evolvedSpec =
+ PartitionSpec.builderFor(table.schema())
+ .withSpecId(1)
+ .bucket("data", 16)
+ .bucket("id", 8, "id_bucket_8")
+ .build();
+ assertThat(table.spec())
+ .as("Should append a partition field to the spec")
+ .isEqualTo(evolvedSpec);
+ assertThat(table.spec().lastAssignedFieldId()).isEqualTo(1001);
+
+ // Restart the spec
+ table
+ .updateSpec()
+ .fromSpec(PartitionSpec.builderFor(table.schema()).build())
+ .addField(bucket("id", 8))
+ .commit();
+
+ V1Assert.assertEquals(
+ "Should soft delete id and data buckets",
Review Comment:
Isn't this incorrect? Don't we keep the id 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