Skip to content

fix: kleene logic bug #1045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
Fokko merged 1 commit into from
Mar 6, 2025
Merged

fix: kleene logic bug #1045

Fokko merged 1 commit into from
Mar 6, 2025

Conversation

@sdd
Copy link
Contributor

@sdd sdd commented Mar 6, 2025

Fixes #1044

@sdd sdd force-pushed the fix/arrow-use-kleene-logic branch from 49f96b0 to 9cc3e34 Compare March 6, 2025 08:22
@sdd sdd marked this pull request as ready for review March 6, 2025 08:23
@sdd sdd force-pushed the fix/arrow-use-kleene-logic branch from 9cc3e34 to a7d4fcf Compare March 6, 2025 08:29
@sdd sdd mentioned this pull request Mar 6, 2025
Closed
3 tasks
Fokko
Fokko approved these changes Mar 6, 2025
View reviewed changes
Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Great catch @sdd, I wasn't aware of this!

crates/iceberg/src/arrow/reader.rs
// Table data: [NULL, "foo", "bar"]
let data_for_col_a = vec![None, Some("foo".to_string()), Some("bar".to_string())];

// Expected: [NULL, "foo"].
Copy link
Contributor

@Fokko Fokko Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the regular or, it drops the null:

assertion `left == right` failed
  left: [Some("foo")]
 right: [None, Some("foo")]

Left:  [Some("foo")]
Right: [None, Some("foo")]

Xuanwo
Xuanwo approved these changes Mar 6, 2025
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @sdd for fixing this!

@Fokko Fokko merged commit d22cf4d into apache:main Mar 6, 2025
17 checks passed
@kevinjqliu kevinjqliu mentioned this pull request Mar 30, 2025
Merged
kevinjqliu added a commit to apache/iceberg-python that referenced this pull request Mar 31, 2025
@kevinjqliu
fix upsert with null values (#1861)
d69a191 
<!--
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.
-->
@kevinjqliu kevinjqliu mentioned this pull request Mar 31, 2025
Open
Fokko pushed a commit to apache/iceberg-python 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.
-->
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

@Xuanwo Xuanwo Xuanwo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Unexpected and and or behaviour when RHS evaluates to null / None

3 participants

@sdd @Fokko @Xuanwo