ibis icon indicating copy to clipboard operation
ibis copied to clipboard

test: fix ArrayValue.remove() handling of NULLs

Open NickCrews opened this issue 9 months ago • 1 comments

ibis.array([1, None, 2]).remove(None) doesn't behave as expected. Still a WIP, figuring out what should be desired.

NickCrews avatar Apr 30 '24 18:04 NickCrews

Sorry, this is very much a WIP, should have marked it as such. Added a very basic description to the OP.

NickCrews avatar Jun 03 '24 19:06 NickCrews

I think the postgres choice of using IS NOT DISTINCT FROM semantics in remove is inconsistent with NULL input behavior in nearly every other function and API.

If an argument is NULL, that means it's unknown and for consistency the only sane thing to do would be to return NULL in such cases.

Now, it's important to have convenience APIs, for example for removing all NULLs in an array.

I propose that we make the semantics of Ibis's Array.remove match those of Trino (which returns NULL when NULL is the element to search for), and add a new API compress (?) that strips all nulls from an array.

cpcloud avatar Jul 23 '24 12:07 cpcloud

@cpcloud just to be clear, currently ibis.array([1, None, 2]).remove(2) results in [1]. In this PR I just want to fix it so this results in [1, None]. Were you thinking I was trying to make Array.remove(None) strip NULLs? I am happy with not supporting that, and expecting users to do Array.filter(_.notnull())

NickCrews avatar Jul 28 '24 07:07 NickCrews

Ah, yep, I thought that's what you had in mind. Glad to see we're in agreement here!

cpcloud avatar Jul 28 '24 10:07 cpcloud

ACTION NEEDED

Ibis follows the Conventional Commits specification for release automation.

The PR title and description are used as the merge commit message.

Please update your PR title and description to match the specification.

github-actions[bot] avatar Jul 28 '24 17:07 github-actions[bot]

Yup that's my bad for a vague PR title and no description (I edited it to be more clear after I saw your confusion, you're not being gaslit here, it WAS confusing)

NickCrews avatar Jul 28 '24 17:07 NickCrews

Thanks @cpcloud for finishing this up!

NickCrews avatar Jul 31 '24 15:07 NickCrews