Skip to content

Conversation

NickCrews
Copy link
Contributor

ops.Literal.value can be None if it's a null literal. We weren't dealing with this case in StructValue.__getitem__.

When I made this fix, it also revealed a different bug when promoting the types of eg ops.Literal(<int>) + ops.Literal(<int>), so I fixed that bug as well.

@github-actions github-actions bot added the tests Issues or PRs related to tests label Jun 3, 2025
@NickCrews
Copy link
Contributor Author

At a larger scale, I think this reveals a very common footgun lurking all over our codebase. If I grep for isinstance\(.*, ops\.Literal\) then I see several other cases where we assume that ops.Literal.value is not None. For example, if you try ibis.to_sql(ibis.array([1,2,3])[ibis.null(int)]) then this errors from comparing None to an int. Perhaps we should consider making a special operation, ops.Null so that ops.Literal.value is never None? But that might just be moving the problem around.

@cpcloud
Copy link
Member

cpcloud commented Jun 4, 2025

I think you're right.

I also think we should merge this change and discuss (perhaps in a separate issue?) a new design for null handling. None simply isn't cutting it anymore, and we need to make it easy to make nulls behave consistently within Ibis.

@cpcloud
Copy link
Member

cpcloud commented Jun 4, 2025

One "fun" aspect of the NULL design will be to decide whether basically all operations are valid on NULLs and they simply return NULL...

@NickCrews
Copy link
Contributor Author

Sounds good to merge this and then make a new issue. I'm curious what larger changes you are thinking of. I haven't come up with any real complaints in terms of the semantics of how nulls behave, I was just thinking of refactors.

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.

Thanks!

@cpcloud cpcloud changed the title fix: deal with null literals in Struct.__getitem__ fix(api): deal with null literals in Struct.__getitem__ Jun 4, 2025
@cpcloud cpcloud merged commit ccd9359 into ibis-project:main Jun 4, 2025
112 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants