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.

Sign up for GitHub

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

Jump to bottom

Json UUID any #1962

Merged
merged 5 commits into from
Oct 2, 2019
Merged

Json UUID any #1962

merged 5 commits into from
Oct 2, 2019

Conversation

ian-r-rose
Copy link
Contributor

Fixes #1944. This does not add support for JSON, JSONB, and UUID PostgreSQL types. Instead, it marks them as having dt.Any type, allowing the tables to be loaded, and those columns mostly ignored.

There is also some black formatting on some lines I didn't touch...

@ian-r-rose ian-r-rose force-pushed the json-uuid-any branch 2 times, most recently from 560fdef to 882a394 Compare September 10, 2019 20:48
@toryhaavik
Copy link
Contributor

hopefully #1963 can get in ahead of this, you can rebase on top of it, and then only your enhancement will be there, not the miscellaneous formatting changes too

@toryhaavik
Copy link
Contributor

@ian-r-rose could you please rebase on top of master? i'll try to take a look at this soon

@ian-r-rose
Copy link
Contributor Author

@toryhaavik Sure thing. I still haven't gotten a decent test working for this, but should be able to take another look this week.

@ian-r-rose ian-r-rose force-pushed the json-uuid-any branch 3 times, most recently from e30b7c7 to 4c9a98b Compare September 17, 2019 22:46
@ian-r-rose
Copy link
Contributor Author

Okay, this is ready for a look. The test failure doesn't seem related to me, but I could be wrong.

@xmnlab
Copy link
Contributor

xmnlab commented Sep 26, 2019

@ian-r-rose I will review that today. if you have a time, could you rebase it on top of master?

@ian-r-rose
Copy link
Contributor Author

Rebased

@xmnlab
Copy link
Contributor

xmnlab commented Sep 27, 2019

@ian-r-rose I was meditating here about this .. and I think the use of Any for that maybe is not the right choice.

@toryhaavik @cpcloud any thought here?

@ian-r-rose
Copy link
Contributor Author

@xmnlab I originally had this as dt.string, but thought that was less appropriate, since that would apply various string methods where they were incorrect. What I like about dt.any is that it allows it to pass through to the client, but doesn't really allow for any operations on it.

In a sense, Any may be the wrong name for the concept, and something like Unknown would make more sense. This is the distinction that TypeScript makes, where any is assignable to anything, but unknown is assignable to nothing without a type cast. See some discussion on this here.

@xmnlab
Copy link
Contributor

xmnlab commented Sep 28, 2019

hey @ian-r-rose I opened a PR to your branch to illustrate my thoughts about this topic.

ian-r-rose#1

I think we probably should create a data type for each of this types you are proposing.
let's wait for the feedback from the other maintainers

again, thanks for working on that :)

xmnlab
xmnlab approved these changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@xmnlab xmnlab left a comment

Choose a reason for hiding this comment

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

LGTM! thanks @ian-r-rose for working on that.

@xmnlab
Copy link
Contributor

xmnlab commented Oct 2, 2019

@toryhaavik if you have time could you review this PR?

also the test for Linux py35 already passed .. but here it didn't updated the status and merge is blocked.

toryhaavik
toryhaavik approved these changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@toryhaavik toryhaavik left a comment

Choose a reason for hiding this comment

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

This LGTM! I'm glad we settled on something closer to the real implementation than Any. i'll try to bump the tests to get them to pass.

@toryhaavik
Copy link
Contributor

i can't get the checks to run. no idea what to do to fix it. @ian-r-rose could you please amend your commit and force push? maybe that'll trigger a clean run of the CI

@ian-r-rose
Copy link
Contributor Author

Sure thing, rebased!

@toryhaavik toryhaavik merged commit e8e1397 into ibis-project:master Oct 2, 2019
@ian-r-rose
Copy link
Contributor Author

Thanks for the reviews and merge @xmnlab and @toryhaavik

costrouc pushed a commit to costrouc/ibis that referenced this pull request Oct 10, 2019
@ian-r-rose @costrouc
Json UUID any
28ad40b 
Fixes ibis-project#1944. This does *not* add support for JSON, JSONB, and UUID
PostgreSQL types. Instead, it marks them as having `dt.Any` type,
allowing the tables to be loaded, and those columns mostly ignored.
There is also some black formatting on some lines I didn't touch...
Author: Ian Rose <[email protected]>
Author: Ivan Ogasawara <[email protected]>

Closes ibis-project#1962 from ian-r-rose/json-uuid-any and squashes the following commits:

6a878b0 [Ian Rose] Merge pull request #1 from Quansight/json-uuid-any
298efc2 [Ivan Ogasawara] Added JSON JSONB and UUID data types.
79bab94 [Ian Rose] Move test to postgres client test suite.
3748ef9 [Ian Rose] Add some light type testing for json, jsonb, uuid.
b514069 [Ian Rose] Allow postgres client to read tables with UUID, JSON, JSONB types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@toryhaavik toryhaavik toryhaavik approved these changes

@xmnlab xmnlab xmnlab approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Degrees of support for new types (JSON, JSONB, UUID)
3 participants
@ian-r-rose @toryhaavik @xmnlab