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

BUG: bool data type not converting, when it should #217

Closed
wants to merge 1 commit into from

Conversation

thequackdaddy
Copy link

Hello,

This resolves (for me at least) #208.

I have a MSSQL table with a bit data column. In odo, we are converting that to bool, which is fine. However, bool types throw an error here. I'm not an expert on what is supposed to be going on here, but this gets it to work for me.

Thanks.

Be a little more careful...
@llllllllll
Copy link
Member

The issue here is that numpy does not have support for Option(bool) so we cannot convert that type. If your dataset has NULL, numpy cannot store that in a bool field.

@thequackdaddy
Copy link
Author

@llllllllll Ahhh...

Should I close this then? Or is there a better way?

Really the usecase is that I have a MSSQL database that I read from with BIT fields. Its probably safe assuming that that when NULL implies False and I'd like odo/blaze to assume as much, even with a warning.

Would the blaze ecosystem be OK with a patch that automatically assumed this, but maybe throw an error? Maybe this is an odo behavior?

@thequackdaddy
Copy link
Author

Actually I may have missed something, but this seems to work now when I have odo=0.5.1. It coerces bits to False when they are NULL which is fine by me.

Closing, and thanks to whoever/whatever fixed this.

@llllllllll
Copy link
Member

Weird, I know there was some work in in that edge but I didn't think the mssql stuff got updated. I am not totally sure I support the current behavior, but only because it makes it easy to not realize that you actually had a bunch of NULLs in your data. I do agree that if you needed to pick, False is a better default, but it is not hard to make it explicit. With blaze you could do: bz.transform(tbl, col=tbl.col.coalesce(False)) which would always do the right thing and be explicit about any conversions.

I don't feel so strongly that I want to go change this right now but I don't know if this was an intended change. Right now the MSSql backend is not run with automated testing so it is hard to make sure the behavior stays the same.

@thequackdaddy
Copy link
Author

@llllllllll I'm happy to attempt to write some testing code, if someone can give hints. Its not obvious to me how to test MSSQL...

I'd appreciate some clues.

@llllllllll
Copy link
Member

We can probably use mostly the same set of tests for postgres or sqlite. What I don't know is how we would get an database available for travis to use.

@thequackdaddy
Copy link
Author

@llllllllll I'd guess the answer w.r.t. travis is "no". From the brief amount of experience I have trying to get a *nix client to talk to MSSQL, its an absolute nightmare.

With that said, appveyor appears to support it, but the question would be would the odo team want to add appveyor testing? I'm aware that appveyor can be very messy...

@llllllllll
Copy link
Member

I have worked with appveyor before and don't find it to be too much of a hassle. I am not sure who has access to enable the github hooks for appveyor for odo but if someone wanted to write an appveyor file I wouldn't mind adding it. I will open an issue on odo and xref this one.

@llllllllll llllllllll mentioned this pull request Dec 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants