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

Ensure that numeric precision is included only if not None #796

Merged
merged 3 commits into from
Jun 16, 2018

Conversation

mjumbewu
Copy link
Contributor

This pull request ensures that the numeric_size of the Column is not None before using including it in the column type string representation.


While unioning tables together with the dbt-utils union_table macro, we were running into syntax errors like the following from SQL generated by DBT:

invalid input syntax for integer: none
LINE 54:            "total_tax_due"::numeric(None) as ...`

We were initially inclined to go and add a precision to the source table, but the models where this was happening were themselves based on derivative tables. For example, in the above case, total_tax_due is defined as an aggregate:

...
sum(tax_due) AS total_tax_due
...

So identifying the precision beforehand is nontrivial.

@drewbanin
Copy link
Contributor

don't try to get too clever
🙄 😞

Thanks @mjumbewu - this looks good to me. I think this is a Postgres-only quirk. On Redshift, numeric will expand to numeric(18, 0). There's a similar issue with text types -- dbt interprets them as varchar(255) whereas it should be unlimited in length on pg.

I think it would be really great to add unit tests for these methods. Is that something you're interested in contributing? If not, I can tack some on here early next week!

Thanks for contributing :)

@mjumbewu
Copy link
Contributor Author

mjumbewu commented Jun 16, 2018

@drewbanin Yes, definitely makes sense. I started down that path, but didn't immediately see a test module to add them to (and was in a bit of a rush 😊). I went ahead and added a test module.

Also thought it'd be nice to see an integration test for this on Postgres. I added a couple numeric fields for the 008_schema_tests_test.py module. I wasn't sure what I needed to run the integration tests locally, so we'll see whether those break anything.

@drewbanin
Copy link
Contributor

perfect! Looks great - I'll merge when appveyor passes :)

Thanks again!

@drewbanin drewbanin merged commit 13691ad into dbt-labs:development Jun 16, 2018
mjumbewu added a commit to stepwise/dbt that referenced this pull request Jun 19, 2018
)

* Ensure that numeric precision is included only if not None

* Add unit test for data_type of field with empty numeric precision

* Add numeric fields to the schema_tests integration tests
@drewbanin drewbanin mentioned this pull request Jul 19, 2018
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