feat(api): move from .case() to .cases() #9096
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NickCrews
force-pushed
the
case-to-cases
branch
5 times, most recently
from
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. |
NickCrews
commented
May 1, 2024
| .else_(nulls) | ||
| .end() | ||
| ) | ||
| return self.cases(*enumerate(labels), else_=nulls) |
There was a problem hiding this comment.
now that this is such a simple implementation I would consider deprecating and then removing the whole .label() API.
NickCrews
force-pushed
the
case-to-cases
branch
6 times, most recently
from
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? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.baseorcases. 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.pyso 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 = NULLnever evals to True. EXCECPT for clickhouse, which appears to special case this. See the addedtest_switch_cases_nulltest. 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.battingtable 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