Replace deprecated adapter methods with relations #133

jtcohen6 · 2019-04-26T21:27:16Z

Fixes 0.13.0 compatibility #129
Closes replaced get_columns_in_table with get_columns_in_relation #132
Some breaking changes to contracts used by helper macros

drewbanin

This looks really great! Some small comments here in the interest of

minimizing end-user confusion as we make these breaking changes
pulling dbt-utils in line with the database-aware functionality provided by dbt

Using relations here is a really good decision - this will cause things like union_tables to automatically become database-aware (with your change). This is especially good because the source() function returns a Relation, and there are lots of really cool use cases you can imagine here!

So, great work and thanks for opening this PR! Happy to discuss any of my comments in here :)

README.md

macros/sql/get_tables_by_prefix.sql

macros/sql/get_tables_by_prefix_sql.sql

drewbanin · 2019-04-27T15:51:47Z

macros/sql/star.sql

    {%- set include_cols = [] %}
-    {%- set cols = adapter.get_columns_in_table(schema_name, table_name) -%}
+    {%- set cols = adapter.get_columns_in_relation(from) -%}


I think there might be merit to adding a check here that returns an error if a non-relation is provided to this macro (and others). This would have the added benefit of raising an error if someone passes an ephemeral model into star.

Maybe:

{% if from.get('name') is none %} {% do exceptions.raise_compiler_error("Macro star() expected a Relation but received the value: " ~ from) %} {% endif %}

or something. We could wrap this up in its own little macro as a helper :)

Good idea! We might add this error to all of the macros that now expect relation arguments in a break-y sort of way. Two q's:

Is there a better way to infer the argument type, or is arg.get('name') != none the best way to confirm that arg is a Relation?

Is there a Jinja method you know of that lets us pass the name of the current macro? Relevant if we were to make error_if_not_relation(arg) its own macro, and wanted to pretty print a dynamic compiler error "Macro " ~ my_macro ~ " expected a Relation for argument " ~ arg"

These are really good questions!

I don't think we have a great mechanism in place to assert the types of objects from the jinja context. We could conceivably add a context function or a jinja test (like abc is Relation) but that doesn't exist today :/. I think our best option is going to be to look for a name attribute. Here's some example code I played around with:

{% set things = [ 'abc.def', api.Relation.create(schema='ghi', identifier='jkl'), ] %} {% for thing in things %} {{ log(thing ~ " is relation? " ~ (thing is mapping and thing.name is defined), info=true) }} {% endfor %}

Let's be sure to test this with dbt-labs/dbt-core#1416

We also don't have a way to get the macro name from the macro context. I think we should just anticipate making a macro to handle raising the exception, then pass in a string to use as the macro name from the caller.

Right on. Is an is_relation() macro something that I should add to utils now, or do you anticipate creating it within the context of dbt proper?

Yeah - let's add it to dbt-utils! Maybe name it something like _is_relation() since I wouldn't be comfortable with folks using this externally (yet). If we had some way of locally scoping the macro I'd suggest that, but we can just indicate its private-ness with a naming convention for now.

Possibly related to the PR you posted: from.name does return a value, but from.get('name') does not. The macro fails the Relation test as written above.

Hmm, yeah, we'll probably want to call from.name then. @beckjake off the top of your head, can you think of a better way to tell if an object is a Relation in the jinja context than obj is mapping and obj.name is not none?

Yeah, .name is a property that actually resolves to .path.get('identifier').

I think I'd do obj is mapping and 'type' in obj.get('metadata', {})). We should probably add a jinja test for this, or something.

Ew, maybe you have to even check the type string:

obj is mapping and obj.get('metadata', {}).get('type', '').endswith('Relation')

Implemented the most rigorous logic in a macro _is_incremental(), which returns the following exception:

Compilation Error Macro star expected a Relation but received the value: hello > in macro _is_relation (macros/cross_db_utils/_is_relation.sql) > called by macro star (macros/sql/star.sql) > called by <Unknown>

README.md

drewbanin · 2019-05-08T01:44:20Z

@jtcohen6 is this ready for a final review?

jtcohen6 · 2019-05-08T13:46:10Z

@drewbanin Yessir!

drewbanin

Ok, this LGTM! Very nice work here @jtcohen6 - thanks so much for digging way into this! Merge when ready :)

drewbanin · 2019-05-08T14:05:18Z

macros/sql/get_tables_by_prefix_sql.sql

@@ -1,25 +1,25 @@
-{% macro get_tables_by_prefix_sql(schema, prefix, exclude='') %}
+{% macro get_tables_by_prefix_sql(schema, prefix, exclude='', database=target.database) %}


I think this will work great, but there's an important thing to know about python (and therefore jinja, I assume?) argument defaults. Check out this article on mutable default arguments.

This code is A-OK because target.database is a string, which is immutable. It would only be a problem if the code read:

{% macro get_tables_by_prefix_sql(schema, prefix, exclude='', databases=[]) %}

or similar.

drewbanin · 2019-05-08T14:08:17Z

macros/sql/get_tables_by_prefix_sql.sql

-            table_schema || '.' || table_name as ref
-        from information_schema.tables
+            table_schema as "table_schema", table_name as "table_name"
+        from {{database}}.information_schema.tables


We spent a lot of time getting this working right with quoting in dbt-core. I think this code is fine for now, but in general, we'll probably want to create some sort of object (analogous to Relations) which encapsulates a database + schema and any applicable quoting configs.

This code won't work if a database is named my-database on eg. Snowflake. While a user could pass in their database string as "my-database", that would cause problems when we create the Relation object in get_tables_by_prefix. I'm ok with shipping this as-is, but let's keep examples like this in mind as we improve dbt's understanding of databases and quoting

Fully agreed. I had tried using the adapter.quote() method (necessary in the BQ version below, given our BQ test project name), but it caused errors on Snowflake. Noted for our future selves.

drewbanin · 2019-05-08T14:13:00Z

macros/cross_db_utils/_is_relation.sql

@@ -0,0 +1,5 @@
+{% macro _is_relation(obj, macro) %}


This is the intended behavior, but it's important to note: This will raise if the ref of an ephemeral model is passed in here. The ref() function returns a string for ephemeral models, vs. a Relation for any non-ephemeral model (or source). I believe all of the macros here that use _is_relation are expecting a table/view, so that is A-OK!

jtcohen6 added 2 commits April 26, 2019 17:25

Replace deprecated adapter methods with relations

e79b19e

Handle snowflake info schema casing

c315cfc

jtcohen6 requested a review from drewbanin April 27, 2019 00:38

drewbanin reviewed Apr 27, 2019

View reviewed changes

drewbanin reviewed Apr 29, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

jtcohen6 added 2 commits April 30, 2019 09:23

Add _is_relation test. Consistent contracts

0ce0b44

Better _is_relation logic

358f94d

drewbanin approved these changes May 8, 2019

View reviewed changes

jtcohen6 merged commit 4c2eb2c into master May 8, 2019

jtcohen6 deleted the fix/use-relations branch May 8, 2019 15:19

emilieschario mentioned this pull request May 8, 2019

get_column_values returns an error if passed a ref, but the model has not been previously built #119

Closed

freyley mentioned this pull request May 8, 2019

Broken 0.10 compatibility from MR133 #134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated adapter methods with relations #133

Replace deprecated adapter methods with relations #133

jtcohen6 commented Apr 26, 2019

drewbanin left a comment

drewbanin Apr 27, 2019

jtcohen6 Apr 29, 2019 •

edited

drewbanin Apr 29, 2019

jtcohen6 Apr 30, 2019

drewbanin Apr 30, 2019

jtcohen6 Apr 30, 2019

drewbanin Apr 30, 2019

beckjake Apr 30, 2019 •

edited

beckjake Apr 30, 2019

jtcohen6 Apr 30, 2019

drewbanin commented May 8, 2019

jtcohen6 commented May 8, 2019

drewbanin left a comment

drewbanin May 8, 2019

drewbanin May 8, 2019

jtcohen6 May 8, 2019

drewbanin May 8, 2019

		@@ -1,25 +1,25 @@
		{% macro get_tables_by_prefix_sql(schema, prefix, exclude='') %}
		{% macro get_tables_by_prefix_sql(schema, prefix, exclude='', database=target.database) %}

Replace deprecated adapter methods with relations #133

Replace deprecated adapter methods with relations #133

Conversation

jtcohen6 commented Apr 26, 2019

drewbanin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 Apr 29, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beckjake Apr 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewbanin commented May 8, 2019

jtcohen6 commented May 8, 2019

drewbanin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtcohen6 Apr 29, 2019 •

edited

beckjake Apr 30, 2019 •

edited