You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/11/24 19:19:14 UTC
[GitHub] [iceberg] islamismailov opened a new pull request, #6268: Allow dropping a column used by an old but not currrent partition spec
islamismailov opened a new pull request, #6268:
URL: https://github.com/apache/iceberg/pull/6268
Adding another unit test to make sure that columns used by an old but not currrent partition spec are allowed to be 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
[GitHub] [iceberg] Fokko merged pull request #6268: Allow dropping a column used by an old but not currrent partition spec
Posted by GitBox <gi...@apache.org>.
Fokko merged PR #6268:
URL: https://github.com/apache/iceberg/pull/6268
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] islamismailov commented on pull request #6268: Allow dropping a column used by an old but not currrent partition spec
Posted by GitBox <gi...@apache.org>.
islamismailov commented on PR #6268:
URL: https://github.com/apache/iceberg/pull/6268#issuecomment-1341261730
(i did address Ryan's feedback)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6268: Allow dropping a column used by an old but not currrent partition spec
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6268:
URL: https://github.com/apache/iceberg/pull/6268#discussion_r1032834941
##########
core/src/test/java/org/apache/iceberg/TestPartitionSpecInfo.java:
##########
@@ -85,6 +85,31 @@ public void testSpecInfoPartitionedTable() {
Assert.assertNull(table.specs().get(Integer.MAX_VALUE));
}
+ @Test
+ public void testColumnDropWithPartitionSpecEvolution() {
+ PartitionSpec spec = PartitionSpec.builderFor(schema).identity("id").build();
+ TestTables.TestTable table = TestTables.create(tableDir, "test", schema, spec, formatVersion);
+
+ Assert.assertEquals(spec, table.spec());
+
+ TableMetadata base = TestTables.readMetadata("test");
+ PartitionSpec newSpec =
+ PartitionSpec.builderFor(table.schema()).identity("data").withSpecId(1).build();
+ table.ops().commit(base, base.updatePartitionSpec(newSpec));
+
+ int initialColSize = table.schema().columns().size();
+ table.updateSchema().deleteColumn("id").commit();
+
+ Assert.assertEquals(newSpec, table.spec());
+ Assert.assertEquals(newSpec, table.specs().get(newSpec.specId()));
+ Assert.assertEquals(spec, table.specs().get(spec.specId()));
+ Assert.assertEquals(
+ ImmutableMap.of(spec.specId(), spec, newSpec.specId(), newSpec), table.specs());
+ Assert.assertNull(table.specs().get(Integer.MAX_VALUE));
+ Assert.assertEquals(
+ "Schema must have one less column", initialColSize - 1, table.schema().columns().size());
Review Comment:
This check could be better if you produced the expected schema and compared. You can use equality comparison for the underlying struct type. Just compare `table.schema().asStruct()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] islamismailov commented on a diff in pull request #6268: Allow dropping a column used by an old but not currrent partition spec
Posted by GitBox <gi...@apache.org>.
islamismailov commented on code in PR #6268:
URL: https://github.com/apache/iceberg/pull/6268#discussion_r1035135510
##########
core/src/test/java/org/apache/iceberg/TestPartitionSpecInfo.java:
##########
@@ -85,6 +85,31 @@ public void testSpecInfoPartitionedTable() {
Assert.assertNull(table.specs().get(Integer.MAX_VALUE));
}
+ @Test
+ public void testColumnDropWithPartitionSpecEvolution() {
+ PartitionSpec spec = PartitionSpec.builderFor(schema).identity("id").build();
+ TestTables.TestTable table = TestTables.create(tableDir, "test", schema, spec, formatVersion);
+
+ Assert.assertEquals(spec, table.spec());
+
+ TableMetadata base = TestTables.readMetadata("test");
+ PartitionSpec newSpec =
+ PartitionSpec.builderFor(table.schema()).identity("data").withSpecId(1).build();
+ table.ops().commit(base, base.updatePartitionSpec(newSpec));
+
+ int initialColSize = table.schema().columns().size();
+ table.updateSchema().deleteColumn("id").commit();
+
+ Assert.assertEquals(newSpec, table.spec());
+ Assert.assertEquals(newSpec, table.specs().get(newSpec.specId()));
+ Assert.assertEquals(spec, table.specs().get(spec.specId()));
+ Assert.assertEquals(
+ ImmutableMap.of(spec.specId(), spec, newSpec.specId(), newSpec), table.specs());
+ Assert.assertNull(table.specs().get(Integer.MAX_VALUE));
+ Assert.assertEquals(
+ "Schema must have one less column", initialColSize - 1, table.schema().columns().size());
Review Comment:
will do
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] islamismailov commented on pull request #6268: Allow dropping a column used by an old but not currrent partition spec
Posted by GitBox <gi...@apache.org>.
islamismailov commented on PR #6268:
URL: https://github.com/apache/iceberg/pull/6268#issuecomment-1340018978
Can somebody stamp this? It's only adding a unit test so is super safe
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org