-
Notifications
You must be signed in to change notification settings - Fork 609
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): support all dtypes in MapGet and MapContains #8648
Conversation
a1b0b83
to
6678570
Compare
EDIT: it looks like these might have been failing for an unrelated reason? rebasing on main with teh maybe fix? @cpcloud what should I do about the failing flink tests in CI? If I run |
@NickCrews When making marker global variables in test modules we write them with lowercase. |
@cpcloud fixed those global variable names. Still am curious if you can help me re: flink above so I can get the flink tests failing locally. |
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.
LGTM, thanks for working through this.
1fe6ae6
to
d5ad0fc
Compare
The mark for risingwave was misleading. It was sometimes combined with the postgres mark, saying the reason was that only string -> string is supported. But that isn't true. I had to get rid of the `raises=PsycoPg2InternalError` in the risingwave mark, because for some tests the error was actually raised by a lack of the FromYMD Operation. pytest marks don't have support for "this error is caused by one of these two reasons", so I have to go with the lowest common denominator, and not list a reason.
This should be O(1) now not O(n). Also it unifies the API so that MapGet and MapContains work more similarly, eg the same dtype combos will work. I was running into this in https://github.com/ibis-project/ibis/actions/runs/8364426602/job/22899760255?pr=8648
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.
This is failing on Snowflake pretty hard.
Clouds are passing:
|
Thanks so much for finishing this one off @cpcloud ! This was a marathon, I started getting tired of figuring this out :) |
Fixes #8605
I bet a lot of these tests will fail in CI. IDK, a lot of these tests feel like overkill. Could just ensure that the Expr can get created, but skip actually executing them on the backend. That requires a lot more boilerplate. But, it is sort of nice to actually have an up-to-date status on what types each backend supports in maps.