Ignore partition fields that are dropped from the current-schema

[Fokko]

A second attempt to fix this (the first attempt in #11604 but that one was not safe).

When a field that's dropped that still part of an older partition spec, we currently get an error. See #11842

This will replace it with the UnknownType which will be read as a null. This will work fine, since when the evaluator encounters a null, it will assume that there is no value:

iceberg/api/src/main/java/org/apache/iceberg/expressions/Projections.java

Lines 204 to 207 in dbd7d1c

	if (parts == null) {
	// the predicate has no partition column
	return Expressions.alwaysTrue();
	}

I misused the UnknownType here, but I think it is better than returning an arbitrary type, like we do in the UnknownTransform:

iceberg/api/src/main/java/org/apache/iceberg/transforms/UnknownTransform.java

Lines 63 to 67 in dbd7d1c

	@Override
	public Type getResultType(Type type) {
	// the actual result type is not known
	return Types.StringType.get();
	}

Unfortunately, my first approach, where we drop the field, is incorrect, since V1 tables rely on the order.

Fixes #4563
Fixes #12738

[github-actions]

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.

[github-actions]

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.

[stevenzwu]

LGTM

[RussellSpitzer]

minor nit, this is testing "ignoring" inactiveFields, they aren't dropped right?

[Fokko]

They are dropped from the struct that's being returned, but ignore is maybe a bit friendlier :)

left a comment to be clarified

[amogh-jahagirdar]

I'm a bit concerned about changing the behavior of this API to bindUnchecked since that skips over validations that still apply when parsing the schema. If I understand the intent,, it's because we're currently failing here

I think I agree that fundamentally when parsing a spec and binding we don't care if a source field exists or not since we'll replace it with unknown later. However, there are some other checks in checkCompatibility that still seem worth running through at this point when the sourceType is not null like making sure that the transform and type does align, the sourcetype isn't something like a struct etc.

It's unfortunately a bit more complicated but should we have a checkCompatibilityIgnoringMissing and a buildIgnoringMissingFields which just does the same checks as checkCompatibility but essentially ignoring if a ifeld is missing, and if it is defined we still do the existing checks? Then we largely keep the same guarantees on this function while solving the issue we set out to solve.

[amogh-jahagirdar]

cc @RussellSpitzer @stevenzwu what do you think?

[stevenzwu]

It's unfortunately a bit more complicated but should we have a checkCompatibilityIgnoringMissing and a buildIgnoringMissingFields which just does the same checks as checkCompatibility but essentially ignoring if a ifeld is missing, and if it is defined we still do the existing checks?

this seems like a reasonable approach. We can also just add a boolean arg to the checkCompatibility method. it is a non-public method.

We still need the strict check of source field for the code path of TableMetadata.addPartitionSpecInternal.

TableMetadataParser is the only place using bindUnchecked. I assume it is for the same reason. Maybe the buildChecked can switch to this trimmed down version of checkCompatibility and ignore inactive source fields.

[Fokko]

The change to buildUnchecked is required because of a check that was added not long ago: #12887, where someone was previously allowed to create an invalid partition spec.

I've updated the code. Let me know what you think. If we don't want to use buildUnchecked, then we need three variations:

• buildStrict when a new PartitionSpec is added, we want to make sure that all the fields are there
• build with allowing missing fields for the inactive specs that contain fields that are dropped.
• bindUnchecked when we don't want to perform checks at all

[Fokko]

TableMetadataParser is the only place using bindUnchecked. I assume it is for the same reason.

This is correct

[amogh-jahagirdar]

Awesome thank you @Fokko I think this is a great fix for a long standing issue that we should get in! Thank you @RussellSpitzer @stevenzwu @HonahX for reviewing!