-
Notifications
You must be signed in to change notification settings - Fork 543
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
feat(api): move from .case() to .cases() #9096
base: main
Are you sure you want to change the base?
Conversation
8ac2d67
to
6885a2c
Compare
EDIT: duh, it's because they don't guarantee row order. Updated the assertions to be order-independent. Any idea as to why the datafusion, exasol, and risingwave column tests are failing? I still have trouble getting those backends running on my M1 so I can't debug locally very well. |
.else_(nulls) | ||
.end() | ||
) | ||
return self.cases(*enumerate(labels), else_=nulls) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that this is such a simple implementation I would consider deprecating and then removing the whole .label()
API.
d3ac95a
to
513bb94
Compare
This is setup for ibis-project#9039, where I change the API of Value.cases(), so I want to make sure that this functionality doesn't change, but the user gets a deprecationwarning
@cpcloud gentle nudge for a review here :) |
@cpcloud anything I can do to help move this forward/easier to review? |
@NickCrews would you mind fixing the conflicts and CI, then I'll nudge the team to get you a review soon. |
Thanks for the poke. I have a lot of other PRs in flight at the moment with ibis, many of which I think are a lot closer to the finish line than this, I'd love to get those merged first before I add another ball to juggle. |
Redo of #7914 (with substantive changes) and #9039 (merely switching the base repo to the correct one, my fork)
Summary of changes:
ibis.null().cases((None, "fill"), else_="not hit")
always results in "not hit". Maybe not the best ergonomics, but at least it is consistent and written down. Perhaps we can revisit later. See one of the TODOs below.base
orcases
. Really it needs to depend on ALL inputs.ibis.cases()
(results in NULL) andibis.cases(else_=5)
(results in 5). I considered disallowing these, but I don't think there is anything semantiically wrong with supporting this.backends/test/test_generic.py
so they are run on all backends.TODOs that I found that should come in followups:
val.substitute({None: 4})
currently does a fillna(). But if you doval.cases((None, 4), else_=val)
, then this ALWAYS hits the else_ case, becausex = NULL
never evals to True. EXCECPT for clickhouse, which appears to special case this. See the addedtest_switch_cases_null
test. This also isn't even consistent in the sense that it only special cases for pythonNone
. If you doibis.null()
, or something only known at runtime likeibis.literal(5).nullif(5)
, then this will always hit the else_ case. Due to these limitations, I vote for making matching against NULLs out of scope for .cases and .substitute. If a user wants to do this, then they better do a .fillna() before.batting
table has a column RBI of type int64. On sqlite, this.to_pandas()
to a column of type object. I have this marked as broken here, but would be good to fix separately.Literal('foo', type=bool)
, should error, but doesn't