Json UUID any by ian-r-rose · Pull Request #1962 · ibis-project/ibis

ian-r-rose · 2019-09-10T19:02:06Z

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...

toryhaavik · 2019-09-11T13:31:51Z

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 · 2019-09-17T15:47:28Z

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

ian-r-rose · 2019-09-17T15:50:11Z

@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 · 2019-09-17T23:17:03Z

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

xmnlab · 2019-09-26T14:52:54Z

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

ian-r-rose · 2019-09-26T15:47:21Z

Rebased

ibis/sql/tests/test_sqlalchemy.py

xmnlab · 2019-09-27T21:59:38Z

@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 · 2019-09-27T22:10:01Z

@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 · 2019-09-28T00:52:47Z

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

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

xmnlab · 2019-10-02T02:19:45Z

@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

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 · 2019-10-02T18:56:39Z

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

JSON JSONB UUID data types

ian-r-rose · 2019-10-02T19:30:56Z

Sure thing, rebased!

ian-r-rose · 2019-10-02T21:10:32Z

Thanks for the reviews and merge @xmnlab and @toryhaavik

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 <ian.r.rose@gmail.com> Author: Ivan Ogasawara <ivan.ogasawara@gmail.com> 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.

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

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

Allow postgres client to read tables with UUID, JSON, JSONB types.

b514069

ian-r-rose force-pushed the json-uuid-any branch from 4c9a98b to 6591a9a Compare September 26, 2019 15:39

Add some light type testing for json, jsonb, uuid.

3748ef9

ian-r-rose force-pushed the json-uuid-any branch from 6591a9a to 3748ef9 Compare September 26, 2019 16:10

xmnlab reviewed Sep 27, 2019

View reviewed changes

ibis/sql/tests/test_sqlalchemy.py Outdated Show resolved Hide resolved

Move test to postgres client test suite.

79bab94

ian-r-rose force-pushed the json-uuid-any branch from 1b59daa to 79bab94 Compare September 27, 2019 16:13

Added JSON JSONB and UUID data types.

298efc2

xmnlab approved these changes Oct 2, 2019

View reviewed changes

toryhaavik approved these changes Oct 2, 2019

View reviewed changes

Merge pull request #1 from Quansight/json-uuid-any

6a878b0

JSON JSONB UUID data types

ian-r-rose force-pushed the json-uuid-any branch from 822f671 to 6a878b0 Compare October 2, 2019 19:30

toryhaavik merged commit e8e1397 into ibis-project:master Oct 2, 2019

Conversation

ian-r-rose commented Sep 10, 2019

Uh oh!

toryhaavik commented Sep 11, 2019

Uh oh!

toryhaavik commented Sep 17, 2019

Uh oh!

ian-r-rose commented Sep 17, 2019

Uh oh!

ian-r-rose commented Sep 17, 2019

Uh oh!

xmnlab commented Sep 26, 2019

Uh oh!

ian-r-rose commented Sep 26, 2019

Uh oh!

Uh oh!

xmnlab commented Sep 27, 2019

Uh oh!

ian-r-rose commented Sep 27, 2019

Uh oh!

xmnlab commented Sep 28, 2019

Uh oh!

xmnlab left a comment

Choose a reason for hiding this comment

Uh oh!

xmnlab commented Oct 2, 2019

Uh oh!

toryhaavik left a comment

Choose a reason for hiding this comment

Uh oh!

toryhaavik commented Oct 2, 2019

Uh oh!

ian-r-rose commented Oct 2, 2019

Uh oh!

ian-r-rose commented Oct 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants