Skip to content

fix upsert with null values#1861

Merged
kevinjqliu merged 3 commits intoapache:mainfrom
kevinjqliu:kevinjqliu/upsert-with-null
Mar 31, 2025
Merged

fix upsert with null values#1861
kevinjqliu merged 3 commits intoapache:mainfrom
kevinjqliu:kevinjqliu/upsert-with-null

Conversation

@kevinjqliu
Copy link
Contributor

@kevinjqliu kevinjqliu commented Mar 30, 2025 •
edited
Loading

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

@kevinjqliu kevinjqliu added this to the PyIceberg 0.9.1 milestone Mar 30, 2025
Fokko
Fokko reviewed Mar 31, 2025
View reviewed changes
@Fokko
Copy link
Contributor

Fokko commented Mar 31, 2025

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

Fokko
Fokko reviewed Mar 31, 2025
View reviewed changes
@kevinjqliu
Copy link
Contributor Author

kevinjqliu commented Mar 31, 2025 •
edited
Loading

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
Copy link
Contributor

Fokko commented Mar 31, 2025 •
edited
Loading

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 kevinjqliu merged commit d69a191 into apache:main Mar 31, 2025
7 checks passed
@kevinjqliu kevinjqliu deleted the kevinjqliu/upsert-with-null branch March 31, 2025 17:33
@kevinjqliu
Copy link
Contributor Author

kevinjqliu commented Mar 31, 2025

thanks for the review @Fokko

i opened #1865 to test the scan logic with nulls

@kevinjqliu kevinjqliu mentioned this pull request Mar 31, 2025
Closed
3 tasks
Fokko pushed a commit that referenced this pull request Apr 17, 2025
@kevinjqliu @Fokko
fix upsert with null values (#1861)
cfb4f25 
<!--
Thanks for opening a pull request!
-->

<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->

# Rationale for this change
Closes #1835

Original implementation, `!=`
([not_equal](https://arrow.apache.org/docs/python/generated/pyarrow.compute.not_equal.html#pyarrow.compute.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

<!-- In the case of user-facing changes, please add the changelog label.
-->
gabeiglio pushed a commit to Netflix/iceberg-python that referenced this pull request Aug 13, 2025
@kevinjqliu @gabeiglio
fix upsert with null values (apache#1861)
b2269c2 
<!--
Thanks for opening a pull request!
-->

<!-- In the case this PR will resolve an issue, please replace
${GITHUB_ISSUE_ID} below with the actual Github issue id. -->
<!-- Closes #${GITHUB_ISSUE_ID} -->

# Rationale for this change
Closes apache#1835

Original implementation, `!=`
([not_equal](https://arrow.apache.org/docs/python/generated/pyarrow.compute.not_equal.html#pyarrow.compute.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

<!-- In the case of user-facing changes, please add the changelog label.
-->
@mdwint mdwint mentioned this pull request Sep 4, 2025
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fokko Fokko Fokko approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

PyIceberg 0.9.1

Development

Successfully merging this pull request may close these issues.

Upsert with NULL value not working

2 participants

@kevinjqliu @Fokko