Core, Spark: Handle unknown type during deletes

[nastra]

Historical background / Problem

There was an issue a while ago where one couldn't drop a partition field and then remove it from a table's schema, as that would always result in an error. This was fixed by #11868, where the field's type was replaced with UnknownType and thus ignored during reads. However, if a user performs a DELETE operation after dropping a partition field and then removing it from a table's schema, the DELETE operation fails in a few places, because those places don't deal with the UnknownType.

Approach

This PR updates all of the places that were failing during the DELETE operation to properly deal with the UnknownType. I'm currently unsure if that's the right approach to deal with this issue, so looking for some feedback/ideas on that. I've also added some tests that reproduce the original issue.

fixes #14343

[nastra]

handling it similar to how it's done in

iceberg/api/src/main/java/org/apache/iceberg/PartitionSpec.java

Lines 136 to 139 in 5ab73bf

	// When the source field has been dropped we cannot determine the type
	if (sourceType == null) {
	resultType = Types.UnknownType.get();
	}

which was added by 8134815

[rdblue]

I think this should default sourceType to UnknownType.get() and still call getResultType. The reason is that for many transforms, the result type is known. For example, bucket always produces IntegerType.get(). That will result in fewer cases where we have an unknown and use Void.class from this.

[nastra]

the lower/upper bound is converted in

iceberg/core/src/main/java/org/apache/iceberg/util/ManifestFileUtil.java

Lines 51 to 52 in 5ab73bf

	this.lowerBound = Conversions.fromByteBuffer(primitive, summary.lowerBound());
	this.upperBound = Conversions.fromByteBuffer(primitive, summary.upperBound());

but we can't convert it to its underlying type, since the type isn't known

[nastra]

the lower/upper bound is converted to a byte buffer in

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

Lines 80 to 81 in c07f2aa

	min != null ? Conversions.toByteBuffer(type, min) : null,
	max != null ? Conversions.toByteBuffer(type, max) : null);

[rdblue]

Seems reasonable to me.

[nastra]

the lower/upper bound is converted from a byte buffer in

iceberg/core/src/main/java/org/apache/iceberg/util/ManifestFileUtil.java

Lines 51 to 52 in 5ab73bf

	this.lowerBound = Conversions.fromByteBuffer(primitive, summary.lowerBound());
	this.upperBound = Conversions.fromByteBuffer(primitive, summary.upperBound());

but we can't convert it to its underlying type, since the type isn't known

[nastra]

otherwise this fails with a CCE since datum can be a String, which is then casted to the NullWriter Void

[rdblue]

Instead, what about just changing the type parameter of NullWriter to Object? The argument is ignored anyway so that avoids the CCE and we don't have to special-case it here.

[nastra]

that would be ideal, but that breaks the API, because ValueWriters.nulls() is public. We could add a deprecation to that method and mention that the return type will be changed to ValueWriters<Object> in the next release, wdyt?

[rdblue]

You can get around this by casting to a raw unparameterized type:

  private static class NullWriter implements ValueWriter<Object> {
    @SuppressWarnings({"unchecked", "rawtypes"})
    private static final ValueWriter<Void> INSTANCE = (ValueWriter) new NullWriter();

    private NullWriter() {}

    @Override
    public void write(Object ignored, Encoder encoder) throws IOException {
      encoder.writeNull();
    }
  }

The resulting ValueWriter<Void> is more restrictive than ValueWriter<Object> so it is safe and you don't have to change the API.

[nastra]

nice, that would also work, thanks

[rdblue]

Should this be moved after the null and nan value checks? The null and nan counts are still valid, right?

This should also have a test update.

[rdblue]

Looks good once the null writer issue is fixed.

[nastra]

thanks for reviewing @rdblue