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

Add Column.is_number/is_float (#1969) #2046

Merged
merged 2 commits into from
Jan 24, 2020
Merged

Conversation

beckjake
Copy link
Contributor

Fixes #1969

A fun question to think about: on snowflake, is nothing an integer, or is everything? Snowflake doesn't technically have non-NUMBER integer types: https://docs.snowflake.net/manuals/sql-reference/data-types-numeric.html#int-integer-bigint-smallint-tinyint-byteint - On the database end, information_schema.columns reports all these as being DECIMAL(38, 0). I've gone with: everything on snowflake is numeric, float, or text and none of it is integer.

@cla-bot cla-bot bot added the cla:yes label Jan 13, 2020
Split out snowflake column type
added column type test integration tests
Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

I want to give this another quick pass, but it broadly looks good to me

plugins/bigquery/dbt/adapters/bigquery/column.py Outdated Show resolved Hide resolved
def is_numeric(self) -> bool:
return self.dtype.lower() in ['numeric', 'number']
return self.dtype.lower() in ['numeric', 'decimal']
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is the only thing in this PR that I'm not sure about -- do you know why number was in here before? I tried looking through the blame but couldn't find the origin.... AFAICT number is not a valid type on pg/redshift/snowflake/bq.....?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only valid on snowflake, and previously this method applied for Snowflake as well as postgres/redshift. That's why I removed it when I split snowflake out.

Copy link
Contributor

@drewbanin drewbanin left a comment

Choose a reason for hiding this comment

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

Ship it!

@beckjake beckjake merged commit 80574a5 into dev/barbara-gittings Jan 24, 2020
@beckjake beckjake deleted the feature/is-number branch January 24, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants