Skip to content

Fixes issues with NULLTYPE not being callable#31

Closed
keiranmraine wants to merge 2 commits intocockroachdb:masterfrom
keiranmraine:feature/bad_null_type
Closed

Fixes issues with NULLTYPE not being callable#31
keiranmraine wants to merge 2 commits intocockroachdb:masterfrom
keiranmraine:feature/bad_null_type

Conversation

@keiranmraine
Copy link
Copy Markdown

This fixes some issues with non-callable fields but then fails to continue due to inability to parse the boolean and timestamp type from table columns:

/.../cockroachdb/sqlalchemy/dialect.py:108: SAWarning: Could not parse type name 'TIMESTAMP WITH TIME ZONE'
  warn("Could not parse type name '%s'" % type_str)
/.../cockroachdb/sqlalchemy/dialect.py:116: SAWarning: Did not recognize type 'BOOL' of column 'is_current'
  (type_name, name))
/.../cockroachdb/sqlalchemy/dialect.py:116: SAWarning: Did not recognize type 'BOOL' of column 'qr_tested'
  (type_name, name))
/.../cockroachdb/sqlalchemy/dialect.py:116: SAWarning: Did not recognize type 'BOOL' of column 'imports_approved'
  (type_name, name))

@keiranmraine
Copy link
Copy Markdown
Author

Corrected the boolean parsing, but not sure how to handle TIMESTAMP WITH TIME ZONE:

SHOW COLUMNS FROM group_component;
+--------------+--------------------------+-------+----------------+-----------+
| Field | Type | Null | Default | Indices |
+--------------+--------------------------+-------+----------------+-----------+
| id | INT | false | unique_rowid() | {primary} |
| id_group | INT | false | NULL | {} |
| id_component | INT | false | NULL | {} |
| created_dt | TIMESTAMP WITH TIME ZONE | false | now() | {} |
| wall_sec | INT | true | NULL | {} |
+--------------+--------------------------+-------+----------------+-----------+

@bdarnell
Copy link
Copy Markdown
Contributor

bdarnell commented Aug 9, 2017

Thanks for the contribution! Can you add some tests that cover these cases?

As for TIMESTAMP WITH TIME ZONE, I think the time zone is handled at a different level so as far as SQLAlchemy's concerned it should be mapped to a plain TIMESTAMP. (This means changing _type_map from dict() with kwarg syntax to a dict literal so it can contain keys with spaces). This is what SQLAlchemy does in its postgres dialect.

Also, it might be a good idea to map both bool and boolean to sqltypes.BOOLEAN. I see [here][(https://github.com/zzzeek/sqlalchemy/blob/eaceeae0f721be72cc589a310e50f0de9f24d721/lib/sqlalchemy/dialects/postgresql/base.py#L1324) that in postgres it's apparently spelled boolean and so we might change CockroachDB to report boolean instead of bool for consistency.

@bdarnell
Copy link
Copy Markdown
Contributor

Fixed in #82.

@bdarnell bdarnell closed this Nov 27, 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.

2 participants