Skip to content
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.

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

fix(api): treat col == None or col == ibis.NA as col.isnull() #9114

Merged
merged 2 commits into from
May 6, 2024

Conversation

jcrist
Copy link
Member

@jcrist jcrist commented May 3, 2024

Previously we coerced co == None to IdenticalTo(col, None), but wouldn't do so for ibis.NA (resulting in that compiling as Equals(col, None) (which would result in incorrect SQL). This PR fixes this eager branch to handle any null literal, and compiles it the same as isnull/notnull (usually col IS NULL) rather than the more verbose col IS NOT DISTINCT FROM NULL that using IdenticalTo would result in.

Other options for this fix would be to:

  • Always return Equals and use a rewrite rule to rewrite these cases. I opted against this since we'd have to have the same detection logic later, and this rewrite rule would be applied for every backend. Honestly having Equals.__new__/NotEquals.__new__ optionally return IsNull/NotNull would be cleaner IMO, but that's harder to do with our operation infrastructure.
  • Always return Equals, and branch when compiling to generate the proper SQL there. This is a bit harder to do, and also would need to be implemented for dataframe backends so col == None would still use the proper isnull call.

@cpcloud
Copy link
Member

cpcloud commented May 4, 2024

How would someone compute a NULL-preserving equality with this change? It seems like it would no longer be possible to do that.

For the semantics you've defined here, you can already do this with a.identical_to(NA).

@cpcloud
Copy link
Member

cpcloud commented May 4, 2024

Oh, it looks like we're doing that already (as you note in the description), nevermind!

Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

Seems like the reversed operand case doesn't work. Looking into it.

@cpcloud cpcloud added this to the 9.1 milestone May 6, 2024
@cpcloud cpcloud enabled auto-merge (squash) May 6, 2024 13:10
@cpcloud cpcloud disabled auto-merge May 6, 2024 13:10
@cpcloud cpcloud enabled auto-merge (squash) May 6, 2024 13:11
@cpcloud cpcloud merged commit 711bf9f into ibis-project:main May 6, 2024
81 checks passed
@jcrist jcrist deleted the fix-eq-na branch May 6, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants