[Bug] Allow users to drop column description in `update_column`

[sungwy]

Apache Iceberg version

0.7.1 (latest release)

Please describe the bug 🐞

Currently, tying to remove an existing description from a column using update_column with either doc=None or doc="" doesn't do anything.

This issue can be attributed to the way we detect the doc value we want to apply on the NestedField.

iceberg-python/pyiceberg/table/__init__.py

Lines 2504 to 2510 in 8e33e55

	self._updates[field.field_id] = NestedField(
	field_id=field.field_id,
	name=field.name,
	field_type=field_type or field.field_type,
	doc=doc or field.doc,
	required=field.required,
	)

doc or field.doc results in field.doc when doc is an empty string.

We need to update the code so that we can support dropping the description.

[corleyma]

This proposed change made me wonder if I may have been implicitly relying on this behavior (bootstrapping tables with a schema, but then ingesting new data that may evolve the schema...)

E.g., I've been doing something like:

  with iceberg_table.update_schema(allow_incompatible_changes=allow_incompatible_changes) as update:
      update.union_by_name(sanitized_pyiceberg_schema)

As I looked into it, it looks like the relevant codepath for this is slightly different, and lives here.
In my opinion, the ideal behavior when unioning would be to take the existing field doc if there's no doc defined for the field in the new schema, otherwise update... and it looks like that is what was being implemented as well in the link above, except I think there's a bug in the condition? shouldn't it be if field.doc is not None and field.doc != existing_field.doc:?

[sungwy]

Hi @corleyma I think you are right! I think both of these code paths have a bug... it would be great to get this fixed together this time around with additional tests demonstrating this use case.

Thank you very much for pointing this out!

[TiansuYu]

In the first code path, the doc or updated.doc ignores doc if doc='', though the behavior of the method should be overwrite. Agree on the second one, should be a bug, the method itself should have merge / union behavior (based on the class name UnionByNameVisitor).

[TiansuYu]

WIP PR #1083