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

fix: replace NaNs with None in some backends when loading from pandas dataframe #9094

Merged
merged 3 commits into from
May 6, 2024

Conversation

chloeh13q
Copy link
Contributor

@chloeh13q chloeh13q commented May 1, 2024

Description of changes

Examples were broken on MySQL backend and PySpark backend when there are null values in a numeric column.

Druid, PySpark, RW don't support examples.

Issues closed

#8792

@chloeh13q chloeh13q changed the title fix: replace NaNs with None in mysql and postgres backends when loading from pandas dataframe fix: replace NaNs with None in some backends when loading from pandas dataframe May 1, 2024
@chloeh13q chloeh13q requested a review from gforsyth May 1, 2024 07:24
Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this in @chloeh13q ! I'm on board with trying this out -- I think for mysql, where nan isn't allowed at all, this is a good solution.
For postgres, we should be a little more specific in the conversion.

I also think this could work for mssql and a few other backends that currently can't load a large chunk of the examples.

ibis/backends/postgres/__init__.py Outdated Show resolved Hide resolved
@chloeh13q
Copy link
Contributor Author

chloeh13q commented May 1, 2024

For postgres, we should be a little more specific in the conversion.

Sounds good.

I also think this could work for mssql and a few other backends that currently can't load a large chunk of the examples.

I think mssql examples are broken for a different reason, at least on my local; I filed a ticket #9095 yesterday and I'm getting Incorrect syntax on all of the examples. Do you have other backends in mind? Or you just meant this in a generic sense

@chloeh13q chloeh13q requested a review from gforsyth May 2, 2024 19:22
@chloeh13q chloeh13q marked this pull request as ready for review May 2, 2024 19:23
@chloeh13q
Copy link
Contributor Author

chloeh13q commented May 2, 2024

I'm having trouble spinning up some of the backends so I'm not able to test whether the examples work in these backends. But I can confirm that the examples are now fixed in MySQL and postgres with this PR!

@gforsyth
Copy link
Member

gforsyth commented May 3, 2024

xref #9110

Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gforsyth gforsyth merged commit f2a7cd9 into ibis-project:main May 6, 2024
82 checks passed
@cpcloud cpcloud added this to the 9.1 milestone May 6, 2024
@cpcloud cpcloud added ux User experience related issues postgres The PostgreSQL backend mysql The MySQL backend labels May 6, 2024
@cpcloud
Copy link
Member

cpcloud commented May 6, 2024

Do we have some tests for this? How do we know this won't regress?

@gforsyth
Copy link
Member

gforsyth commented May 6, 2024

We should enable some of the examples tests for postgres and mysql (I don't think we need to run all of them)

gforsyth added a commit to gforsyth/ibis that referenced this pull request May 6, 2024
The palmer penguins data has null valued integers, which when used to
create a `memtable`, leads to pandas casting a column to `float` because
it reads in the nulls as NaN.

We added a fix for this for some backends in ibis-project#9094.
gforsyth added a commit that referenced this pull request May 6, 2024
The palmer penguins data has null valued integers, which when used to
create a `memtable`, leads to pandas casting a column to `float` because
it reads in the nulls as NaN.

We added a fix for this for some backends in #9094.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mysql The MySQL backend postgres The PostgreSQL backend ux User experience related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants