You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "anuragmantri (via GitHub)" <gi...@apache.org> on 2024/12/20 23:17:44 UTC
[PR] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
anuragmantri opened a new pull request, #11842:
URL: https://github.com/apache/iceberg/pull/11842
Prevents
- https://github.com/apache/iceberg/issues/10234
- https://github.com/apache/iceberg/issues/10626
- https://github.com/apache/iceberg/issues/11314
- and any other similar reports.
Supersedes: https://github.com/apache/iceberg/pull/10352
Co-authored by: @amogh-jahagirdar
**Summary**
If we drop a partition column that is still referenced in an older spec, this will break the table. This can be easily reproduced in Spark by adding a SELECT after these tests
- https://github.com/apache/iceberg/blob/dea2fd1d9debfd23aeda9403ed3eb81c6aebf30f/spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java#L533
- https://github.com/apache/iceberg/blob/dea2fd1d9debfd23aeda9403ed3eb81c6aebf30f/spark/v3.3/spark-extensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java#L544
```
sql("SELECT * FROM %s LIMIT 1", tableName);
```
Will throw
```
Type cannot be null
java.lang.NullPointerException: Type cannot be null
at org.apache.iceberg.relocated.com.google.common.base.Preconditions.checkNotNull(Preconditions.java:922)
at org.apache.iceberg.types.Types$NestedField.<init>(Types.java:615)
at org.apache.iceberg.types.Types$NestedField.optional(Types.java:508)
at org.apache.iceberg.PartitionSpec.partitionType(PartitionSpec.java:134)
at org.apache.iceberg.Partitioning.buildPartitionProjectionType(Partitioning.java:284)
at org.apache.iceberg.Partitioning.partitionType(Partitioning.java:242)
at org.apache.iceberg.spark.source.SparkTable.metadataColumns(SparkTable.java:258)
```
As discussed by @amogh-jahagirdar and @advancedxy [here](https://github.com/apache/iceberg/pull/10352#discussion_r1676213273), the solution is two parts
Part 1: Prevent dropping the column referenced in older specs (This PR)
Part 2: Provide a maintenance API for users to remove all unused specs (https://github.com/apache/iceberg/pull/10755)
Both can be merged independently.
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2557887971
I will fix other partition evolution tests
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on code in PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#discussion_r1894504885
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -533,6 +534,25 @@ private static Schema applyChanges(
}
}
+ if (base != null) {
+ for (int fieldIdToDelete : deletes) {
+ base.specsById()
+ .forEach(
+ (specId, spec) -> {
+ // Prevent dropping fields that are referenced in older partitions specs in use.
+ if (!specId.equals(base.defaultSpecId())) {
Review Comment:
Wondering if we will run into the issue again if we allow dropping from current 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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #11842: Core: Prevent dropping column which is referenced by active partition specs
URL: https://github.com/apache/iceberg/pull/11842
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560179927
Thanks for your review @advancedxy. My concern is that this is a big behavior change. Partition columns cannot be dropped anymore. I will start a thread in dev channel to see what others think.
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560708474
> So, we cannot alter existing partition specs. Even after upgrading to V2, the metadata is still in V1. The relevant part of the spec:
I think we should be able to evolve the partition spec after upgrading to V2? The relevant part of the spec is for V1 tables, so any implementation when dealing with V1 tables, it should evolve the partition specs in that way. Once the table is upgraded to V2, I think old clients(ones that support v1 tables only) will be rejected. For any relative new clients(supports v2 format), it should produces specs with field id included.
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560827832
> Just to double check, with dropping the offending column, I was assuming that you would mutate an existing spec.
No, I'm not proposing to mutate an existing spec directly. After going to v2, we need to update table's current spec to remove the void transform, in that way, new data files will be produced without the void transform. After all the old data files are rewrote, the spec with void transform could be expired and the source column can be then dropped.
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2611274274
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#discussion_r1896278567
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -533,6 +534,25 @@ private static Schema applyChanges(
}
}
+ if (base != null) {
+ for (int fieldIdToDelete : deletes) {
+ base.specsById()
+ .forEach(
+ (specId, spec) -> {
+ // Prevent dropping fields that are referenced in older partitions specs in use.
+ if (!specId.equals(base.defaultSpecId())) {
Review Comment:
> This makes sense. My concern is that this is a big behavior change. Users cannot drop partition columns at all.
I don't users can drop partition source field/columns(please note , not the partition columns) in current spec. See the following test:
```java
TestTables.TestTable table =
TestTables.create(tableDir, "test", SCHEMA, BY_DATA_SPEC, V1_FORMAT_VERSION);
table.updateSchema().deleteColumn("data").commit();
```
will throws:
```
Cannot find source column for partition field: 1000: data: identity(2)
org.apache.iceberg.exceptions.ValidationException: Cannot find source column for partition field: 1000: data: identity(2)
at org.apache.iceberg.exceptions.ValidationException.check(ValidationException.java:49)
at org.apache.iceberg.PartitionSpec.checkCompatibility(PartitionSpec.java:636)
at org.apache.iceberg.TableMetadata$Builder.build(TableMetadata.java:1469)
at org.apache.iceberg.TableMetadata.updateSchema(TableMetadata.java:560)
at org.apache.iceberg.SchemaUpdate.commit(SchemaUpdate.java:447)
at org.apache.iceberg.TestPartitioning.testDropPartitionSourceField(TestPartitioning.java:94)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:766)
at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:156)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:147)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:86)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(InterceptingExecutableInvoker.java:103)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.lambda$invoke$0(InterceptingExecutableInvoker.java:93)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:92)
at org.junit.jupiter.engine.execution.InterceptingExecutableInvoker.invoke(InterceptingExecutableInvoker.java:86)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$8(TestMethodTestDescriptor.java:217)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:213)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:138)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:68)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:156)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:146)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:144)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:143)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:100)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:160)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:146)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:144)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:143)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:100)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:160)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:146)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:144)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:143)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:100)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:107)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:88)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:54)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:67)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:52)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.processAllTestClasses(JUnitPlatformTestClassProcessor.java:124)
at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor$CollectAllTestClassesExecutor.access$000(JUnitPlatformTestClassProcessor.java:99)
at org.gradle.api.internal.tasks.testing.junitplatform.JUnitPlatformTestClassProcessor.stop(JUnitPlatformTestClassProcessor.java:94)
at org.gradle.api.internal.tasks.testing.SuiteTestClassProcessor.stop(SuiteTestClassProcessor.java:63)
at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
at java.base/java.lang.reflect.Method.invoke(Method.java:580)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:36)
at org.gradle.internal.dispatch.ReflectionDispatch.dispatch(ReflectionDispatch.java:24)
at org.gradle.internal.dispatch.ContextClassLoaderDispatch.dispatch(ContextClassLoaderDispatch.java:33)
at org.gradle.internal.dispatch.ProxyDispatchAdapter$DispatchingInvocationHandler.invoke(ProxyDispatchAdapter.java:92)
at jdk.proxy1/jdk.proxy1.$Proxy4.stop(Unknown Source)
at org.gradle.api.internal.tasks.testing.worker.TestWorker$3.run(TestWorker.java:200)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.executeAndMaintainThreadName(TestWorker.java:132)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:103)
at org.gradle.api.internal.tasks.testing.worker.TestWorker.execute(TestWorker.java:63)
at org.gradle.process.internal.worker.child.ActionExecutionWorker.execute(ActionExecutionWorker.java:56)
at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:121)
at org.gradle.process.internal.worker.child.SystemApplicationClassLoaderWorker.call(SystemApplicationClassLoaderWorker.java:71)
at worker.org.gradle.process.internal.worker.GradleWorkerMain.run(GradleWorkerMain.java:69)
at worker.org.gradle.process.internal.worker.GradleWorkerMain.main(GradleWorkerMain.java:74)
```
So, it make sense to not allow dropping partition source columns in other active partition specs as well. The only problem is that the void transform in V1 table, which still references a source column.
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560794468
Just to double check, with dropping the offending column, I was assuming that you would mutate an existing spec. But I think after going to V2, we should rewrite it into a new spec (that drops the column).
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2625998224
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2557962815
For v1 tables, it's possible that the partition specs are evolved and have a void transform in the spec. After this PR, the source field in the void transform cannot be removed either. We have two options:
1. upgrade the v1 table to v2 and then remove the void transform in the old spec and produces a new one. Call ExpireSnapshot with metadata clean when all the data files written by old spec are rewritten/cleaned after ttl. After that, the v1 specs are expired and the offending columns can be removed.
2. Allow removing the partition source field in void transform in this PR. I think it requires extra work and we may need to keep the input type for void transform, which might lead to a table spec change.
I'd prefer option 1 even though it's a bit inconvenient for users.
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2652867208
The PR is superseded by https://github.com/apache/iceberg/pull/11868
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560684459
@advancedxy Sorry for ignoring comment 1, I had to think about that one a bit:
> upgrade the v1 table to v2 and then remove the void transform in the old spec and produces a new one. Call ExpireSnapshot with metadata clean when all the data files written by old spec are rewritten/cleaned after ttl. After that, the v1 specs are expired and the offending columns can be removed.
So, we cannot alter existing partition specs. Even after upgrading to V2, the metadata is still in V1. The relevant part of the spec:
<img width="783" alt="image" src="https://github.com/user-attachments/assets/044e5081-8e95-4ec2-a515-a78899c30633" />
This means they are still sequential, and we cannot rely on field-IDs. Removing one field in the middle is dangerous since you alter the order. Best case it will throw an error when evaluating the data (if the types are compatible), worst case it will return faulty data. I think when it finds a partition field, that does not point to a field anymore because it is dropped, it should just be ignored when the partition-spec is evaluated.
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560242110
> Allow removing the partition source field in void transform in this PR. I think it requires extra work and we may need to keep the input type for void transform, which might lead to a table spec change.
This is not safe, unfortunately. I was working on a solution for this (see https://github.com/apache/iceberg/pull/11604), but that's also unsafe for V1 tables. I'm planning to work on a different solution that I have in mind.
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560713627
> For any relative new clients(supports v2 format), it should produces specs with field id included.
It should indeed, but you cannot guarantee that, and it is not enforced 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.
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on code in PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#discussion_r1896047271
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -533,6 +534,25 @@ private static Schema applyChanges(
}
}
+ if (base != null) {
+ for (int fieldIdToDelete : deletes) {
+ base.specsById()
+ .forEach(
+ (specId, spec) -> {
+ // Prevent dropping fields that are referenced in older partitions specs in use.
+ if (!specId.equals(base.defaultSpecId())) {
Review Comment:
This makes sense. My concern is that this is a big behavior change. Users cannot drop partition columns at all.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#discussion_r1894534414
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -508,7 +508,8 @@ private static Schema applyChanges(
Multimap<Integer, Types.NestedField> adds,
Multimap<Integer, Move> moves,
Set<String> identifierFieldNames,
- boolean caseSensitive) {
+ boolean caseSensitive,
+ TableMetadata base) {
Review Comment:
Nit: maybe just passing `Map<Integer, PartitionSpec> specsById` here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "anuragmantri (via GitHub)" <gi...@apache.org>.
anuragmantri commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2557871692
@amogh-jahagirdar, @advancedxy @RussellSpitzer - Could 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.
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#discussion_r1894534190
##########
core/src/main/java/org/apache/iceberg/SchemaUpdate.java:
##########
@@ -533,6 +534,25 @@ private static Schema applyChanges(
}
}
+ if (base != null) {
+ for (int fieldIdToDelete : deletes) {
+ base.specsById()
+ .forEach(
+ (specId, spec) -> {
+ // Prevent dropping fields that are referenced in older partitions specs in use.
+ if (!specId.equals(base.defaultSpecId())) {
Review Comment:
Although, dropping partition source columns from the current spec will fail later when building table metadata.
I don't think we should allow that here.
We should remove the if condition here as we certainly cannot remove partition source columns in the current 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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560529630
> > Allow removing the partition source field in void transform in this PR. I think it requires extra work and we may need to keep the input type for void transform, which might lead to a table spec change.
>
> This is not safe, unfortunately. I was working on a solution for this (see #11604), but that's also unsafe for V1 tables. I'm planning to work on a different solution that I have in mind.
Looking forward for your solution. BTW, what do you think about the option 1, it might some additional work from users, but I think it should be doable?
--
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] Core: Prevent dropping column which is referenced by active partition specs [iceberg]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on PR #11842:
URL: https://github.com/apache/iceberg/pull/11842#issuecomment-2560757725
> > For any relative new clients(supports v2 format), it should produces specs with field id included.
>
> It should indeed, but you cannot guarantee that, and it is not enforced by the spec.
Hmmm, maybe I'm not following here. When it's upgraded to v2, I think the table metadata should be regenerated with partition field id included? And the spec enforces that the field id should be unique across all the specs:
https://iceberg.apache.org/spec/#partitioning
<img width="826" alt="image" src="https://github.com/user-attachments/assets/5ef520da-8059-4d9a-8b38-ceda5f3aa6b6" />
--
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