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.

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 · 2024-10-08T19:32:53Z

@cpcloud looks like the docs CI is failing just for permissions issues, so I think this is good to merge! (If you confirm then in the future I will be confident to just merge if I see only this CI failure)

cpcloud · 2024-10-09T09:00:20Z

Please do not merge anything that has CI failures.

This job in particular is testing whether the documentation can be built, and that definitely is a hard requirement for merging a PR.

ibis/expr/types/generic.py

cpcloud · 2024-10-09T13:21:40Z

I put up #10290 to address the docs build failure.

cpcloud · 2024-10-09T13:35:16Z

docs/posts/ci-analysis/index.qmd

-        .else_("NA")
-        .end()
-    ),
+    improvements=_.raw_improvements.cases(


Most of the files in this directory have pre-computed outputs, and if you change them, they'll rerun.

In this case, this causes a failure, because it's trying to authenticate to BigQuery (I'm guessing browser-based auth is first in the credential chain, and thus the message about a missing browser).

Even if the job did have permission to render this quarto document, it would still fail, because we verify that for documents that are "frozen" that they aren't being re-rendered unintentionally.

When stuff like this fails, it's not something that should really ever just be merged, hence the required property of the docs build check.

OK, so the lesson for me is, if I change something in docs/posts/, I should re-run it locally before pushing. for this bigquery notebook, I won't be able to do this since I don't have creds, but for the rest I should be good to go

Fixes ibis-project#7280

NickCrews · 2024-10-09T16:31:12Z

@cpcloud I added the breaking change label to this PR, since it does hard-break Value.cases(), as described in the OP at the top.

cpcloud · 2024-10-09T18:18:50Z

Ah oops. Well, we'll have to add it manually after the release as it's already merged.

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

cpcloud approved these changes Oct 7, 2024

View reviewed changes

NickCrews force-pushed the case-to-cases branch from de1ebc9 to b2e527d Compare October 7, 2024 19:12

github-actions bot added docs Documentation related issues or PRs tests Issues or PRs related to tests impala The Apache Impala backend clickhouse The ClickHouse backend snowflake The Snowflake backend labels Oct 7, 2024

NickCrews force-pushed the case-to-cases branch 3 times, most recently from 7e28c8a to 8956150 Compare October 8, 2024 18:56

cpcloud reviewed Oct 9, 2024

View reviewed changes

ibis/expr/types/generic.py Outdated Show resolved Hide resolved

cpcloud force-pushed the case-to-cases branch 2 times, most recently from 32d20da to aad95b8 Compare October 9, 2024 09:24

cpcloud added this to the 10.0 milestone Oct 9, 2024

cpcloud force-pushed the case-to-cases branch 2 times, most recently from 99ecc43 to be79239 Compare October 9, 2024 09:57

cpcloud added the deprecation Issues or PRs related to deprecating APIs label Oct 9, 2024

cpcloud force-pushed the case-to-cases branch from be79239 to 65ee6e1 Compare October 9, 2024 12:07

cpcloud reviewed Oct 9, 2024

View reviewed changes

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

74a10af

Fixes ibis-project#7280

cpcloud force-pushed the case-to-cases branch from 65ee6e1 to 74a10af Compare October 9, 2024 14:02

cpcloud merged commit 54889db into ibis-project:main Oct 9, 2024
90 checks passed

NickCrews added the breaking change Changes that introduce an API break at any level label Oct 9, 2024

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

NickCrews commented Sep 11, 2024

NickCrews commented Sep 11, 2024

NickCrews commented Oct 8, 2024 •

edited

Loading

cpcloud commented Oct 9, 2024 •

edited

Loading

cpcloud commented Oct 9, 2024

cpcloud Oct 9, 2024

NickCrews Oct 9, 2024

NickCrews commented Oct 9, 2024

cpcloud commented Oct 9, 2024

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

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

NickCrews commented Sep 11, 2024

NickCrews commented Sep 11, 2024

NickCrews commented Oct 8, 2024 • edited Loading

cpcloud commented Oct 9, 2024 • edited Loading

cpcloud commented Oct 9, 2024

cpcloud Oct 9, 2024

Choose a reason for hiding this comment

NickCrews Oct 9, 2024

Choose a reason for hiding this comment

NickCrews commented Oct 9, 2024

cpcloud commented Oct 9, 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

NickCrews commented Oct 8, 2024 •

edited

Loading

cpcloud commented Oct 9, 2024 •

edited

Loading