feat(api): move from .case() to .cases() #9096

NickCrews · 2024-05-01T17:01:37Z

Redo of #7914 (with substantive changes) and #9039 (merely switching the base repo to the correct one, my fork)

I will try to keep the summary below up to date with what this PR does:

Breaking change to `Value.cases()`

Before the API was

def cases(
        self,
        case_result_pairs: Iterable[tuple[ir.BooleanValue, Value]],
        default: Value | None = None,
    ) -> Value:

Now it is

def cases(self, *branches: tuple[Value, Value], else_=None) -> Value:

In other words, it is now variadic, and the else_ is kwarg. The else_ naming convention is now consistent with Value.substitute()

This will be a hard breaking change for users when they upgrade to 10.0.

Alternatives considered (I am currently going for the simplest but harshest path):

Some gnarly compact code that detects and translates old-style use of the API, and emits a warning. This was deemed too complex.
Some slightly less gnarly code that merely detects the old-style API, and errors.

Soft-deprecates `ibis.case()` and `Value.case()`

Existing users will be able to continue to use these APIs with no problems. There will be no warnings or anything else. However, we will remove these functions from the docs, and make the docstring advise to use the new API.
I want there to be ONE endorsed way of doing these conditionals, so I think only advertising one API is very important to me.

Alternatives considered:

Hard-deprecation: Add @util.deprecated(instead="use ibis.cases() instead", as_of="10.0") to these functions.
Hard-removal: remove them entirely in 10.0
Still possible to do: hide .case() even more from users and tab completion, eg behind __getattr__()

See this comment thread for more discussion.

NickCrews · 2024-05-01T17:52:43Z

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.

ibis/expr/types/numeric.py

NickCrews · 2024-05-11T00:09:42Z

@cpcloud gentle nudge for a review here :)

NickCrews · 2024-05-21T21:13:20Z

@cpcloud anything I can do to help move this forward/easier to review?

ncclementi · 2024-06-10T20:11:53Z

@NickCrews would you mind fixing the conflicts and CI, then I'll nudge the team to get you a review soon.

NickCrews · 2024-06-12T18:07:22Z

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.

NickCrews · 2024-06-27T22:42:40Z

One thing that should get merged first is #9334, where I fix a datashape bug that is exposed in this PR. Once that is in, then this PR will be simpler.

cpcloud

Thanks for pushing through!

I still have many concerns about the way things are being done in this PR. There also seem to be a few scope increases (e.g., the cast insertion) that should really be avoided.

ibis/backends/pandas/executor.py

ibis/backends/sql/compiler.py

ibis/backends/tests/test_generic.py

ibis/expr/operations/generic.py

ibis/expr/types/generic.py

cpcloud · 2024-07-01T12:51:23Z

ibis/expr/types/generic.py

-        """
-        import ibis.expr.builders as bl
+    @staticmethod
+    def _norm_cases_args(*args, **kwargs):


I am very opposed to this function. Instead of massaging this to be something close to maintainable, why not just wait until 10.0? Doesn't seem worth the trouble to me.

Yeah I would love to drop this. I was just trying to provide an upgrade path. Ideally:

a user upgrades to 9.2. They now get a deprecation warning, but their code continues to work. There is another API available they can switch to without the deprecation warning.

They upgrade to 10.0. The old API disappears

Without this compat gnarliness, then

a user upgrades to 9.2 They see no change

they upgrade to 10.0. Their code breaks, and won't work until they switch over to the new API.

As a user I definitely prefer the first experience, but if we provide nice actionable error messages then the second experience is definitely bearable. Should we go for the 2nd experience?

Similarly, should we keep the old CaseBuilder stuff around, or should we also hard-delete it all at once and provide an actionable error message?

Also it's good to separate out the possible breaking changes. This is what I propose (which I think you agree with IIUC):

ibis.case(): we add the deprecation in 10.0, add a test that the old API raises a deprecation but still gives the right result. This isn't too hard to continue support. We fully remove in 11.0

Value.case(): Same as above

Value.cases(): remove this _norm_cases_args() gnarliness. This will just be hard breaking change for users when they upgrade 9.x to 10.0. No warning or upgrade path, sorry folks.

Yes, that is correct.

I will look into making these changes in this PR.

In the commit I just pushed, I think we end up with the behavior above:

removed the _norm_cases_args() gnarliness

kept the deprecation warnings for ibis.case() and Value.case(). I also am fine with removing the deprecation, letting past users keep using them with no problem, but just not surfacing them in the docs or otherwise advertising them.

ibis/expr/types/generic.py

ibis/expr/types/numeric.py

This is pinning down the expected behavior for cases before tackling the case() to cases() switch in ibis-project#9096 so that PR can be simpler

Prepping for ibis-project#9096 - moves several .ifelse() and .substitute() tests from ibis/backens/test/test_generic.py into a new test_conditionals.py - moves some .case() tests from the pandas and dask backends into there, so the other backends are now tested as well, and removed duplication. - adds a test case that `ibis.literal(5).nullif(5).case().when(None, "oops").else_("expected")` results in "expected". Not saying this is actually the best behavior, but it is currently what we claim to be expected. Perhaps we want to revisit.

This is pinning down the expected behavior for cases before tackling the case() to cases() switch in ibis-project#9096 so that PR can be simpler

This is pinning down the expected behavior for cases before tackling the case() to cases() switch in ibis-project#9096 so that PR can be simpler I move the validation for comparable-ness down into the operation so that the logic is consolidated to one place. in ibis-project#9096 there might be multiple places that construct an ops.SimpleCase, and we don't want to have to implement the validation in all calling locations. We could consider relaxing the limitation for non-empty cases later, but for now lets be strict.

cpcloud

Still too much going on here. See review comments.

ibis/backends/tests/test_generic.py

cpcloud · 2024-07-16T13:41:06Z

ibis/expr/operations/generic.py

+
+        for case in cases:
+            if not rlz.comparable(base, case):
+                raise TypeError(


Any changes that affect validation and/or type checking or construction of operations need to go in separately from API changes. If you want to make this change please put it in a separate PR.

ibis/backends/sql/compiler.py

This is pinning down the expected behavior for cases before tackling the case() to cases() switch in ibis-project#9096 so that PR can be simpler I move the validation for comparable-ness down into the operation so that the logic is consolidated to one place. in ibis-project#9096 there might be multiple places that construct an ops.SimpleCase, and we don't want to have to implement the validation in all calling locations. We could consider relaxing the limitation for non-empty cases later, but for now lets be strict.

This is pinning down the expected behavior for cases before tackling the case() to cases() switch in ibis-project#9096 so that PR can be simpler I move the validation for comparable-ness down into the operation so that the logic is consolidated to one place. in ibis-project#9096 there might be multiple places that construct an ops.SimpleCase, and we don't want to have to implement the validation in all calling locations. We could consider relaxing the limitation for non-empty cases later, but for now lets be strict. I already fixed the shape of ops.SearchedCase in ibis-project#9334, but it looks like in that PR I forgot to also fix ops.SimpleCase, so I do that fix here.

NickCrews · 2024-07-16T17:55:34Z

Once #9559 lands then I think the scope of this will be significantly reduced and this should be much easier, thank you for your reviews!

This is pinning down the expected behavior for cases before tackling the case() to cases() switch in ibis-project#9096 so that PR can be simpler I move the validation for comparable-ness down into the operation so that the logic is consolidated to one place. in ibis-project#9096 there might be multiple places that construct an ops.SimpleCase, and we don't want to have to implement the validation in all calling locations. We could consider relaxing the limitation for non-empty cases later, but for now lets be strict. I already fixed the shape of ops.SearchedCase in ibis-project#9334, but it looks like in that PR I forgot to also fix ops.SimpleCase, so I do that fix here.

NickCrews · 2024-07-24T20:21:54Z

OK, this is much simpler now, there are no functionality changes, only API changes.

The one question in the air is what to do with the old APIS. Can you respond in the thread and then I can adjust things?

jcrist · 2024-08-12T18:00:43Z

Going through our PR backlog - @cpcloud, can you please give this another review when you have a moment?

ibis/tests/expr/test_case.py

cpcloud · 2024-08-25T15:28:56Z

Thinking about this PR some more, I'm not sure why we're going to all this trouble.

Do we really need to remove the existing case API? What problems is it causing or what additional maintenance does it add?

Right now cases is implemented in terms of the more primitive case, which is much less complex than what is being proposed here.

I think I am now -1 on this PR altogether. It seems like it might be fine to break cases and make it variadic for 10, but removing case seems wholly unecessary.

NickCrews · 2024-08-25T16:20:38Z

I want the user experience to be consistent, and only one of the APIs to be surfaced in the docs and all the examples. I think I'd be fine not deleting the old API, and not even deprecating it, but just removing it from the docs. Also maybe removing it from tab complete? By putting it under gettattr or something?? In terms of maintenance cost I agree its not bad.

I do want to think about this not in terms of the cost to do the transition between these two states, which is a one time thing, and more in terms of how does the quality of the final state compare to how it was before for the user?

Also, if we're worried about breaking people, I believe it is a thing where we can ship a script that goes through their source code and rewrites the change for them (for the easy instances at least). There's a project that does this I think, just blanking on the name

Fixes ibis-project#7280

NickCrews · 2024-09-11T19:05:43Z

I'm still motivated to get this merged. I am trying to keep the original post at the top of this PR in sync with the changes, to provide a good summary of the changes, why we are doing them, and alternatives considered. I think I have mitigated some of your concerns above. Please read that and see what you think? Thanks!

NickCrews · 2024-09-11T19:06:11Z

@cpcloud gentle ping, whenever you get the chance :)

NickCrews mentioned this pull request May 1, 2024

feat(api): move from .case() to .cases() #9039

Closed

NickCrews force-pushed the case-to-cases branch 5 times, most recently from 8ac2d67 to 6885a2c Compare May 1, 2024 17:51

NickCrews commented May 1, 2024

View reviewed changes

ibis/expr/types/numeric.py Show resolved Hide resolved

NickCrews requested a review from cpcloud May 1, 2024 18:05

NickCrews force-pushed the case-to-cases branch 7 times, most recently from 513bb94 to ae70dc6 Compare May 8, 2024 22:11

ncclementi mentioned this pull request Jun 10, 2024

[meta] Ibis expression API stability #8996

Closed

10 tasks

cpcloud added this to the 9.2 milestone Jun 13, 2024

NickCrews mentioned this pull request Jun 27, 2024

fix(datatypes): infer the correct shape for many existing ops #9334

Merged

cpcloud requested changes Jul 1, 2024

View reviewed changes

NickCrews force-pushed the case-to-cases branch 2 times, most recently from c33860b to e0fa647 Compare July 12, 2024 17:09

NickCrews mentioned this pull request Jul 12, 2024

fix(case): fix dshape, error on noncomparable and empty cases #9559

Merged

NickCrews mentioned this pull request Jul 12, 2024

chore(test): consolidate ifelse() and case() tests #9560

Merged

cpcloud requested changes Jul 16, 2024

View reviewed changes

cpcloud removed this from the 9.2 milestone Jul 16, 2024

NickCrews mentioned this pull request Jul 16, 2024

feat: remove IntegerValue.label() #9612

Open

ncclementi mentioned this pull request Jul 19, 2024

[meta]: Ibis expression and backend stability #9638

Open

13 tasks

NickCrews force-pushed the case-to-cases branch 2 times, most recently from 666d98b to 3ccca68 Compare July 24, 2024 20:08

NickCrews requested a review from cpcloud July 24, 2024 20:22

cpcloud reviewed Aug 25, 2024

View reviewed changes

ibis/tests/expr/test_case.py Outdated Show resolved Hide resolved

NickCrews force-pushed the case-to-cases branch 2 times, most recently from 5ed9645 to db52a77 Compare September 11, 2024 18:39

feat: move from .case() to .cases()

de1ebc9

Fixes ibis-project#7280

NickCrews force-pushed the case-to-cases branch from db52a77 to de1ebc9 Compare September 11, 2024 19:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): move from .case() to .cases() #9096

feat(api): move from .case() to .cases() #9096

NickCrews commented May 1, 2024 •

edited

Loading

NickCrews commented May 1, 2024 •

edited

Loading

NickCrews commented May 11, 2024

NickCrews commented May 21, 2024

ncclementi commented Jun 10, 2024

NickCrews commented Jun 12, 2024

NickCrews commented Jun 27, 2024

cpcloud left a comment

cpcloud Jul 1, 2024

NickCrews Jul 12, 2024

NickCrews Jul 24, 2024

cpcloud Aug 25, 2024

NickCrews Sep 11, 2024

cpcloud left a comment

cpcloud Jul 16, 2024

NickCrews commented Jul 16, 2024

NickCrews commented Jul 24, 2024

jcrist commented Aug 12, 2024

cpcloud commented Aug 25, 2024

NickCrews commented Aug 25, 2024

NickCrews commented Sep 11, 2024

NickCrews commented Sep 11, 2024

feat(api): move from .case() to .cases() #9096

Are you sure you want to change the base?

feat(api): move from .case() to .cases() #9096

Conversation

NickCrews commented May 1, 2024 • edited Loading

Breaking change to Value.cases()

Soft-deprecates ibis.case() and Value.case()

NickCrews commented May 1, 2024 • edited Loading

NickCrews commented May 11, 2024

NickCrews commented May 21, 2024

ncclementi commented Jun 10, 2024

NickCrews commented Jun 12, 2024

NickCrews commented Jun 27, 2024

cpcloud left a comment

Choose a reason for hiding this comment

cpcloud Jul 1, 2024

Choose a reason for hiding this comment

NickCrews Jul 12, 2024

Choose a reason for hiding this comment

NickCrews Jul 24, 2024

Choose a reason for hiding this comment

cpcloud Aug 25, 2024

Choose a reason for hiding this comment

NickCrews Sep 11, 2024

Choose a reason for hiding this comment

cpcloud left a comment

Choose a reason for hiding this comment

cpcloud Jul 16, 2024

Choose a reason for hiding this comment

NickCrews commented Jul 16, 2024

NickCrews commented Jul 24, 2024

jcrist commented Aug 12, 2024

cpcloud commented Aug 25, 2024

NickCrews commented Aug 25, 2024

NickCrews commented Sep 11, 2024

NickCrews commented Sep 11, 2024

NickCrews commented May 1, 2024 •

edited

Loading

Breaking change to `Value.cases()`

Soft-deprecates `ibis.case()` and `Value.case()`

NickCrews commented May 1, 2024 •

edited

Loading