fix upsert with null values

[kevinjqliu]

Rationale for this change

Closes #1835

Original implementation, != (not_equal) does not account for null values. It emits null when either sides are null
The new implementation, first checks for not_equal. And on null values, returns true only if both sides are null

Similar to apache/iceberg-rust#1045

Are these changes tested?

Yes

Are there any user-facing changes?

No

[Fokko]

Thanks for looking into this @kevinjqliu, pretty annoying null-handling here :/

[kevinjqliu]

now im wondering if nulls are properly handled when we convert iceberg expressions to pyarrow expressions

iceberg-python/pyiceberg/io/pyarrow.py

Lines 721 to 725 in 1a5e32a

	def visit_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> pc.Expression:
	return pc.field(term.ref().field.name) == _convert_scalar(literal.value, term.ref().field.field_type)
	def visit_not_equal(self, term: BoundTerm[Any], literal: Literal[Any]) -> pc.Expression:
	return pc.field(term.ref().field.name) != _convert_scalar(literal.value, term.ref().field.field_type)

https://arrow.apache.org/docs/python/generated/pyarrow.compute.equal.html

Compare values for equality (x == y).

A null on either side emits a null comparison result.

[Fokko]

now im wondering if nulls are properly handled when we convert iceberg expressions to pyarrow expressions

I think we need to check the Or and And BooleanExpressions, because I think we also want to use the kleene or there 👍

Edit: I think that situation is different since we're comparing two columns in the Upsert logic. For the visitor that you linked, we compare against a literal.

[kevinjqliu]

thanks for the review @Fokko

i opened #1865 to test the scan logic with nulls