Skip to content
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: remove .case(), move to .cases() #7914

Closed
wants to merge 1 commit into from

Conversation

NickCrews
Copy link
Contributor

Redo of #7319 targeting main instead of master

Fixes #7280

This is still WIP for getting all tests to pass, but putting something concrete out here for feedback. I would love you initial thoughts here.

TODO:

  • core tests pass
  • backend tests pass
  • document

Questions:

  • Don't remove .case(), just deprecate it?
  • don't even hard deprecate it, merely soft-deprecate it by removing from examples and docs? It's not that hard to transition to the new API, but it also wouldn't be that hard for us to stealthily continue supporting the old API.
  • rename SearchedCase to just Case, since SimpleCase is now gone?
  • make deferreds on non-Table values work: t.values.cases((_ > 5), "big"), else_="small"). This doesn't work currently either, so we're not moving backwards.
  • The generated SQL isn't as idiomatic for some dialects (eg duckdb) when you pass in values. Eg t.values.cases((5, "five"))` generates
CASE
    WHEN values = 5 THEN "five"

instead of what it used to:

CASE values
   WHEN 5 THEN "five"

IDK if this has any performance penalty (the previous SQL looks ripe for a hash lookup, the new SQL does not, but IDK duckdb is probably smart enough to get around this)

@cpcloud
Copy link
Member

cpcloud commented Mar 21, 2024

@NickCrews How would you like to proceed here?

@cpcloud cpcloud marked this pull request as draft March 21, 2024 12:57
@NickCrews
Copy link
Contributor Author

I still think this is a better API and would like it to happen. I think I understand the internals of ibis a bit better so I can maybe self address some of the problems I mentioned above. I want to get all the other open PRa in before I restart this though. Are you supportive of this change in general? What would be your hard requirements?

@NickCrews
Copy link
Contributor Author

superseded by #9039

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: consolidate ibis.case(), Value.case(), Value.cases(), Value.substitute()
2 participants