ibis icon indicating copy to clipboard operation
ibis copied to clipboard

feat(api): make ibis.literal() idempotent

Open NickCrews opened this issue 1 year ago • 2 comments

Is your feature request related to a problem?

Inspired by https://github.com/ibis-project/ibis/pull/9458#issuecomment-2197316190

I occasionally "just want an ibis expression, I'm not sure what I was handed". For example, in the above issue, when trying to construct an ops.Array value, the members handed to me might be python literals, or ibis expressions. I want to cast them to the datatype I want, so they need to be ibis values, I can't do that casting on python vals.

This is similar in spirit to my current PRs which make ibis.map(), ibis.array(), and ibis.struct() idempotent.

What is the motivation behind your request?

No response

Describe the solution you'd like

def literal(value, type):
    if isinstance(value, (ibis.Expr, ibis.Deferred)):
        return value
    # the rest of the existing logic below

Currently, we explicitly disallow ibis.Exprs as inputs: https://github.com/ibis-project/ibis/blob/b150635cd492d29cafe5c8c9c9544f746e20c8ee/ibis/expr/types/generic.py#L2385-L2394

so presumably there is a reason this hasn't been done yet? Any recollection what the reason was?

What version of ibis are you running?

main

What backend(s) are you using, if any?

No response

Code of Conduct

  • [X] I agree to follow this project's Code of Conduct

NickCrews avatar Jun 28 '24 17:06 NickCrews

I occasionally "just want an ibis expression, I'm not sure what I was handed"

ibis.literal is not the API for that. Does it really make sense to pass arbitrary expressions to ibis.literal?

cpcloud avatar Jun 28 '24 18:06 cpcloud

I agree the name "literal" isn't great, I'm not wedded to stuffing that functionality in there, but is there some other good way of solving my problem? I just updated my OP with a link to a real implementation that I think could benefit from this functionality. IDK, maybe there is a way to avoid needing this functionality all together.

I think I understand your concern with stuffing too much functionality into one place, but I feel like when it is just adding idempotency, that is a lot less to worry about as opposed to eg accepting xarray values or dask values?

NickCrews avatar Jun 29 '24 03:06 NickCrews

Is it a big lift to just keep a function around that does this in whatever code you're writing that needs this behavior?

cpcloud avatar Jul 03 '24 17:07 cpcloud

Going to close this as unplanned for now, @NickCrews -- I think we can keep the concerns in the linked PR scoped to arrays, maps, and structs.

gforsyth avatar Jul 17 '24 17:07 gforsyth