Core: Only validate the current partition specs #5707

Fokko · 2022-09-05T11:51:24Z

If a field is being deleted that used to be part of a partition spec, that will throw an error because it cannot resolve the field anymore. See the issue for more details.

Closes #5676

singhpk234 · 2022-09-05T12:48:57Z

core/src/main/java/org/apache/iceberg/TableMetadataParser.java

+        if (unboundSpec.specId() == defaultSpecId) {
+          builder.add(unboundSpec.bind(schema));


[doubt] as per my understanding, I think this might not work with v1 partition spec considering we still have void transform for dropped partitions, which would be holding reference to dropped source field.

Should we update PartitionSpec#checkCompatibility to handle void transforms. Thoughts ?

also should we add a unit test to TableMetadataParserTest?

@singhpk234 Can you elaborate a bit more? I'm not sure that I understand the issue.

For V1, using the spec instead of specs, nothing changes. Since it takes the other branch:

iceberg/core/src/main/java/org/apache/iceberg/TableMetadataParser.java

Lines 393 to 402 in 24d5a53

Preconditions.checkArgument(

formatVersion == 1, "%s must exist in format v%s", PARTITION_SPECS, formatVersion);

// partition spec is required for older readers, but is always set to the default if the spec

// array is set. it is only used to default the spec map is missing, indicating that the

// table metadata was written by an older writer.

defaultSpecId = TableMetadata.INITIAL_SPEC_ID;

specs =

ImmutableList.of(

PartitionSpecParser.fromJsonFields(

schema, TableMetadata.INITIAL_SPEC_ID, JsonUtil.get(PARTITION_SPEC, node)));

Apologies for not being clear,
What I meant here was, mapping the current schema to the current spec might still cause issue as in case of tables of V1 format, (Considering the UT in this PR, last DDL) The current spec will have a void transform for day_of_ts since day_of_ts is dropped from partitioning (in V1 format, we replace this transform with a void transform). Now when we attempt to bind current partition spec with current schema, current schema will not have day_of_ts, but in PartitionSpec#checkCompatibility we will try to find schema.findType(field.sourceId()), from current schema for void transform of day_of_ts and it will fail, as we have removed day_of_ts from current schema.

A sample UT for repro (modified from this PR) :

@Test public void testDropColumnOfOldPartitionField() { // default table created in v1 format sql( "CREATE TABLE %s (id bigint NOT NULL, ts timestamp, day_of_ts date) USING iceberg PARTITIONED BY (day_of_ts)", tableName); sql("ALTER TABLE %s REPLACE PARTITION FIELD day_of_ts WITH days(ts)", tableName); sql("ALTER TABLE %s DROP COLUMN day_of_ts", tableName); }

@singhpk234 Thanks for the thorough explanation. I wasn't aware of the replacement by a void transform, and it indeed has a similar issue:

Thanks for raising this, and let me think of a solution. Do you know the historical reason to replace it with a void transform?

@Fokko , This explanation from @rdblue, nicely explains the motivation behind introducing void transforms in v1 format.

Thanks for linking @singhpk234, and that is indeed a very clear explanation and it makes a lot of sense.

I've added a condition to skip the validation when we encounter a VoidTransform. Since it is almost always compatible, I think we should be okay with that, but curious to hear what others think.

If a fields is being deleted that used to be part of a partition spec, that will throw an error because it cannot resolve the field anymore. Closes apache#5676 Closes apache#5707 Closes apache#5399

marton-bod · 2022-09-23T11:13:23Z

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java

+  public void testDropColumnOfOldPartitionFieldV1() {
+    // default table created in v1 format
+    sql(
+        "CREATE TABLE %s (id bigint NOT NULL, ts timestamp, day_of_ts date) USING iceberg PARTITIONED BY (day_of_ts)",


Shall we explicitly set format-version=1, in case the default changes in the future?

Great suggestion, added! 👍🏻

@Fokko, it doesn't look like this change was pushed?

I was working on a follow-up to fix the other issue in the read-path. I waited with pushing until I had a proper fix for this but turns out that it is a bit more complicated than I originally anticipated. I've created a new PR #5907

marton-bod · 2022-09-23T11:14:41Z

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java

+
+    sql("ALTER TABLE %s REPLACE PARTITION FIELD day_of_ts WITH days(ts)", tableName);
+
+    sql("ALTER TABLE %s DROP COLUMN day_of_ts", tableName);


Can we also include a sql to read back the data from the table after the drop? We have noticed the same issue in Trino, but for us the drop column still succeeds, it's the subsequent read operations that start failing

Thanks @marton-bod, the ALTER TABLE threw an exception before, but you are right that subsequent reads are also failing. Digging into this

Hmm, Interesting. This is what I assumed would happen and went with spec change (disruptive change)

@marton-bod: I have tagged you in one of the slack discussions. Where I proposed spec change to handle this.
If Fokko doesn't find an easy way to fix this, we can discuss my approach after his experiment.

rdblue · 2022-09-30T23:15:41Z

...ensions/src/test/java/org/apache/iceberg/spark/extensions/TestAlterTablePartitionFields.java

@@ -421,6 +421,31 @@ public void testSparkTableAddDropPartitions() throws Exception {
        "spark table partition should be empty", 0, sparkTable().partitioning().length);
  }

+  @Test
+  public void testDropColumnOfOldPartitionFieldV1() {


I don't think this is the right place to test this. There are partition spec evolution tests in core, which is a much better place than here.

rdblue

I think that this looks good as a first step. We should also add a test or two to exercise the read path and see if anything needs to be fixed there.

rdblue · 2022-09-30T23:19:55Z

Thanks for fixing this, @Fokko! I like this version of the fix.

If a fields is being deleted that used to be part of a partition spec, that will throw an error because it cannot resolve the field anymore. Closes apache#5676 Closes apache#5707 Closes apache#5399

hashhar · 2024-03-06T06:52:10Z

after this change while you can drop partition columns such tables become broken if they hadn't been optimized before dropping the column. Basically see #4563

github-actions bot added core spark labels Sep 5, 2022

Fokko mentioned this pull request Sep 5, 2022

Core: Dropping an old partition column causes NPE (and corrupt metadata on v2 tables) #5676

Closed

singhpk234 reviewed Sep 5, 2022

View reviewed changes

Fokko force-pushed the fd-only-validate-spec-for-current-spec branch 2 times, most recently from 24d5a53 to 4f7db1f Compare September 11, 2022 17:33

github-actions bot added the API label Sep 11, 2022

Fokko mentioned this pull request Sep 21, 2022

Error happened after deleting a partitioned column #5399

Closed

Core: Only validate the current partition specs

2844525

If a fields is being deleted that used to be part of a partition spec, that will throw an error because it cannot resolve the field anymore. Closes apache#5676 Closes apache#5707 Closes apache#5399

Fokko force-pushed the fd-only-validate-spec-for-current-spec branch from 4f7db1f to 2844525 Compare September 21, 2022 12:54

ajantha-bhat mentioned this pull request Sep 22, 2022

API: Drop column of deleted partitioned field to Unbound partitionSpec #4602

Closed

marton-bod reviewed Sep 23, 2022

View reviewed changes

rdblue reviewed Sep 30, 2022

View reviewed changes

rdblue approved these changes Sep 30, 2022

View reviewed changes

rdblue merged commit 3b65cca into apache:master Sep 30, 2022

Fokko deleted the fd-only-validate-spec-for-current-spec branch October 1, 2022 18:18

Fokko mentioned this pull request Oct 3, 2022

Spark: Set the version explicitly #5907

Merged

nastra mentioned this pull request Oct 4, 2022

[1.0.x] Core: Only validate the current partition specs (#5707) #5915

Closed

hashhar mentioned this pull request Sep 11, 2024

ALTER TABLE ... DROP COLUMN allows dropping a column used by old PartitionSpecs #4563

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Only validate the current partition specs #5707

Core: Only validate the current partition specs #5707

Fokko commented Sep 5, 2022

singhpk234 Sep 5, 2022

nastra Sep 6, 2022

Fokko Sep 6, 2022

singhpk234 Sep 6, 2022 •

edited

Loading

Fokko Sep 7, 2022 •

edited

Loading

singhpk234 Sep 7, 2022

Fokko Sep 11, 2022

marton-bod Sep 23, 2022

Fokko Sep 24, 2022

rdblue Sep 30, 2022

Fokko Oct 3, 2022

marton-bod Sep 23, 2022

Fokko Sep 24, 2022

ajantha-bhat Sep 26, 2022

rdblue Sep 30, 2022

rdblue left a comment

rdblue commented Sep 30, 2022

hashhar commented Mar 6, 2024

		if (unboundSpec.specId() == defaultSpecId) {
		builder.add(unboundSpec.bind(schema));

	Preconditions.checkArgument(
	formatVersion == 1, "%s must exist in format v%s", PARTITION_SPECS, formatVersion);
	// partition spec is required for older readers, but is always set to the default if the spec
	// array is set. it is only used to default the spec map is missing, indicating that the
	// table metadata was written by an older writer.
	defaultSpecId = TableMetadata.INITIAL_SPEC_ID;
	specs =
	ImmutableList.of(
	PartitionSpecParser.fromJsonFields(
	schema, TableMetadata.INITIAL_SPEC_ID, JsonUtil.get(PARTITION_SPEC, node)));


		sql("ALTER TABLE %s REPLACE PARTITION FIELD day_of_ts WITH days(ts)", tableName);

		sql("ALTER TABLE %s DROP COLUMN day_of_ts", tableName);

Core: Only validate the current partition specs #5707

Core: Only validate the current partition specs #5707

Conversation

Fokko commented Sep 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

singhpk234 Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

Fokko Sep 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue left a comment

Choose a reason for hiding this comment

rdblue commented Sep 30, 2022

hashhar commented Mar 6, 2024

singhpk234 Sep 6, 2022 •

edited

Loading

Fokko Sep 7, 2022 •

edited

Loading