Skip to content

Add more types to mapping dictionary#74

Merged
bdarnell merged 12 commits intocockroachdb:masterfrom
ckoehn:feature/add-more-types
Jan 23, 2019
Merged

Add more types to mapping dictionary#74
bdarnell merged 12 commits intocockroachdb:masterfrom
ckoehn:feature/add-more-types

Conversation

@ckoehn
Copy link
Copy Markdown
Contributor

@ckoehn ckoehn commented Jan 8, 2019

This PR adds more types to the mapping dictionary (See cockroach docs). It will probably fix #73, fix #63 and fix #60.

Additional note:
I was unsure how to add the ARRAY.

@ckoehn
Copy link
Copy Markdown
Contributor Author

ckoehn commented Jan 8, 2019

I noticed the alpha version of CockroachDB (v2.1.0-alpha.20180730) in the test stage and changed it to v2.1.3 (latest stable) (see #75). 9 Tests were failing and I wanted to see if this PR actually fixes some of the failing tests. We are down to just 3. Will look into them tomorrow.

Any kind of feedback is highly appreciated.

@ckoehn
Copy link
Copy Markdown
Contributor Author

ckoehn commented Jan 9, 2019

I incorporated some changes from #72 (@bdarnell) and all tests are green now ;)

@gigatexal
Copy link
Copy Markdown

Is there an ETA on when this will get merged?

@ckoehn
Copy link
Copy Markdown
Contributor Author

ckoehn commented Jan 10, 2019

We should probably add tests for all these type mappings...

@couchand
Copy link
Copy Markdown

@ckoehn this is great, thanks for working on this.

As you suggest (and the comment above the type map says), it does seem like it would be a good idea to add some tests for these various mappings. Would it make sense to add that to test_introspection.py?

The Cockroach database creates a UNIQUE INDEX implicitly whenever the
UNIQUE CONSTRAINT construct is used. Currently we are just ignoring all
unique indexes, but we might need to return them and add an additional
key `duplicates_constraint` if it is detected as mirroring a constraint.

Reference:
https://www.cockroachlabs.com/docs/stable/unique.html
https://github.com/sqlalchemy/sqlalchemy/blob/55f930ef3d4e60bed02a2dad16e331fe42cfd12b/lib/sqlalchemy/dialects/postgresql/base.py#L723
@knz
Copy link
Copy Markdown
Contributor

knz commented Jan 18, 2019

I could review this but:

  • I want to review this commit by commit. It's not clear yet (to me) we want all of these changes as-is.
  • unfortunatley due to the lack of good reviewing facility in github reviewing the sub-commits effectively is difficult.

Therefore I would kindly request that you send separate PRs for the changes.

@ckoehn
Copy link
Copy Markdown
Contributor Author

ckoehn commented Jan 19, 2019

Thanks for feedback.
You are right. I possibly put too many changes into this one. I will try to split them into multiple PRs this weekend.

Copy link
Copy Markdown
Contributor

@bdarnell bdarnell left a comment

Choose a reason for hiding this comment

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

All of this looks good to me; I think we do want all these changes (note that a lot of the commits are closely related - even though there are 11 commits, there are fewer logical changes here).

I think this mainly just needs to be updated for the conflict in dialect.py, and maybe add some tests to ensure that we have some coverage of the newly-added data types.

…types

* upstream/master:
  Update sqlalchemy for information_schema changes
@bdarnell bdarnell merged commit 2303b1a into cockroachdb:master Jan 23, 2019
@ckoehn ckoehn deleted the feature/add-more-types branch January 23, 2019 21:26
@ckoehn ckoehn mentioned this pull request Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Could not parse type name 'character varying' Support UUID type in the sqlalchemy dialect Parsing 'TIMESTAMP WITH TIME ZONE'

5 participants