clean up relation usage and default behavior #92

dataders · 2021-01-26T19:39:40Z

changes

implements a relation.py which sets the relation:
- quoting policy to both:
  - use " to quote relation parts, and
- inclusion policy to never include the database name when referencing a relation, just schema.identifier
fixes: clean up relation construction #91 cleaner relation name gen ( {{ relation }} instead of {{ relation.schema }}.{{ relation.identifier }})
use adapter.drop_relation which deprecates sqlserver__drop_relation_script
re-enables the snapshot_strategy_check_cols test for Azure SQL (should have been re-enabled with Fix for passing the snapshot adapter test. Fixes #61 #74)

dataders · 2021-01-26T19:54:27Z

@jtcohen6 the import is failing but I have no idea why, but I'm willing to bet it's just a rookie python typo.

dbt/adapters/sqlserver/__init__.py

Co-authored-by: Jeremy Cohen <[email protected]>

…to relation-construction

dataders · 2021-02-11T00:38:54Z

issue

something weird is happening with drop_relation(). For all 8 steps of the empty sequence and the first 12 steps of the base test sequence, the drop_relation's work as they should in that

this jinja-sql

if object_id ('{{ relation }}','{{ object_id_type }}') is not null
    begin
    drop {{ relation.type }} {{ relation }}
    end

compiles to this

 if object_id ('dbt_test_azure_sql_210211000737938430161027.view_model__dbt_backup','V') is not null
      begin
      drop view dbt_test_azure_sql_210211000737938430161027.view_model__dbt_backup
      end

However, it fails starting with step 13 of the base test sequence (running the swappable.sql as incremental). Now the compiled drop_relation SQL looks like this:

if object_id (''dbt_test_azure_sql_210211000737938430161027'.'swappable'','V') is not null
      begin
      drop view 'dbt_test_azure_sql_210211000737938430161027'.'swappable'
      end

@jtcohen6, does the global_project's incremental materialization (and others) change the QuotePolicy somehow? wild guess -- does the { this } relation object inherit the adapter's quoting policy?

SQLServerQuotePolicy is currently set to False for parts, so I don't get why the swappable view is getting quoted here.

@dataclass
class SQLServerQuotePolicy(Policy):
    database: bool = False
    schema: bool = False
    identifier: bool = False

desired behavior

TL;DR I'd like '{{ relation }}' to be rendered as one of these three things:

'database.schema.identifier' per SQLServerQuotePolicy, and
'''database''.''schema''.''identifier''' (inexplicably, ' is the default escape character in TSQL, instead of \)

side note, square brackets are the defacto TSQL way to quote relation parts and column names, but not sure if that's possible to set SQLServerRelation's quote_character to be two characters: [ or ].

jtcohen6

@jtcohen6, does the global_project's incremental materialization (and others) change the QuotePolicy somehow? wild guess -- does the {{ this }} relation object inherit the adapter's quoting policy?

Hm... the {{ this }} relation object definitely inherits the adapter's quoting policy. It also doesn't seem like {{ this }} is directly involved here. To be clear, dbt-sqlserver reimplements the incremental materialization, so I'd think that this line is performing the bad drop (because swappable indeed exists_not_as_table, it exists as a view). I'm not sure what's causing it to quote old_relation, though. Feels like your best bet is to take this for a spin locally—create a model as a view, switch it to incremental, add a lot of logging between lines 17 and 32 of the incremental materialization, and see when exactly it starts being quoted.

While on this subject, it's also worth checking to see if the default incremental materialization still needs overriding—the only difference I can tell is explicitly writing {{ tmp_relation.include(schema=False, database=False) }}, rather than implicitly with {{ tmp_relation }} as the default does, even though the result (at least on dbt-postgres) is exactly the same.

jtcohen6 · 2021-02-11T10:09:53Z

dbt/include/sqlserver/macros/adapters.sql

@@ -145,18 +132,18 @@
   type='view')-%}
   {%- set temp_view_sql = sql.replace("'", "''") -%}

-   {{ sqlserver__drop_relation_script(tmp_relation) }}
+   {{ sqlserver__drop_relation(tmp_relation) }}


I believe this, and the other instances below, can just be {{ adapter.drop_relation(tmp_relation) }}

good catch! qq: why adapter.drop_relation() and not just drop_relation()? is it to make sure the macro can't be overwritten by a macro in the project that is also named drop_relation?

…sft/dbt-sqlserver into relation-construction

dataders · 2021-02-11T17:04:55Z

it's also worth checking to see if the default incremental materialization still needs overriding

@jtcohen6 I've pulled in changes from #102 which drops the overriding of the incremental materialization. Unfortunately, the same issue is still happening, so I'm going to start locally logging/debugging the global project's incremental materialization.

dataders · 2021-02-15T09:01:48Z

dbt/adapters/sqlserver/relation.py

+class SQLServerRelation(BaseRelation):
+    quote_policy: SQLServerQuotePolicy = SQLServerQuotePolicy()
+    include_policy: SQLServerIncludePolicy = SQLServerIncludePolicy()
+    quote_character: str = '"'


@mikaelene this is the one thing I'm not sure about. can you forsee any issues using " instead of ' to quote relation parts?

I am not a guru in quoting :-). Did we have ' before?

Yeah we did, inexplicably though, the code change when I change it back to single quote. You're right that we should think about this change maybe breaking existing snapshot or incremental tables...

I will definitely have to change this back to include database=True by default to make cross-database selects work.

mikaelene

The code look really good. There is a lot of changes. How well have you tested this? Have you run it on a real dbt-project or just in the CI?

dataders · 2021-02-15T10:09:31Z

Have you run it on a real dbt-project or just in the CI?

Great question. I actually just tried running a project that was previously run with (I believe) dbt-sqlserver==0.19.0. And we get an error. So we should def table this PR until 0.19.1, instead of cramming it prematurely into 0.19.0.1.

I haven't seen this error message before, maybe @jtcohen6 could explain. It seems it has to do with partial matches due to quoting differences? this might constitute a breaking change for users. What I don't understand is why:

it searches for an unquoted version, but
finds a quoted version, when
the quote policy is none, however
in all my debugging the relation was always showing up double-quoted (despite the False quote policy)

Compilation Error in model SAPHRPeopleDim (models/stage/SAPHRPeopleDim.sql)
  When searching for a relation, dbt found an approximate match. Instead of guessing 
  which relation to use, dbt will move on. Please delete "ajs_stg"."SAPHRPeopleDim", or rename it to be less ambiguous.
  Searched for: ajs_stg.saphrpeopledim
  Found: "ajs_stg"."SAPHRPeopleDim"
  
  > in macro materialization_view_default (macros/materializations/view/view.sql)
  > called by model SAPHRPeopleDim (models/stage/SAPHRPeopleDim.sql)

jtcohen6 · 2021-02-15T10:34:56Z

I haven't seen this error message before, maybe @jtcohen6 could explain. It seems it has to do with partial matches due to quoting differences? this might constitute a breaking change for users.

Yes, that's right. If dbt finds an existing relation in the database that is similar to a model's relation-to-e, but not exactly the same (different quoting/casing), dbt will raise a compilation error to avoid accidentally overwriting a view/table it doesn't actually own. Here's the source for that behavior.

It sounds like you ran this same project previously, using dbt-sqlserver==0.19.0, when dbt was quoting relations. So, now that quoting has been turned off, dbt has identified the discrepancy. I agree this is a breaking change and should probably wait until v0.20.0.

in all my debugging the relation was always showing up double-quoted (despite the False quote poli

This is the part I'm not sure about! If the quote_policy is now False across the board, dbt should not be quoting relation components. Which brings me to ask: What's the actual reason to turn off quoting on SQLServer? I think I missed this part of the puzzle. The reason we turn it off for Snowflake is because, if an identifier name is quoted and contains non-uppercase characters, selecting it is a nightmare.

never quote any part by default (but I see zero evidence that changing this makes a difference?)

The reason to keep quoting is that, on several databases (e.g. Postgres), identifier names that start with certain characters must be quoted: dev_jerco."123Table". On other databases (e.g. Snowflake), quoting adds so much hassle that it just isn't worth the support for special characters. Some databases (e.g. BigQuery) don't ever support those kind of special-character identifier names, even when quoted.

…to relation-construction

dataders · 2021-02-17T09:58:41Z

What's the actual reason to turn off quoting on SQLServer?

great question @jtcohen6. Looks like there wasn't a good reason and I just did it to try and solve a bug that I stirred up along the way. It isn't necessary. It is however necessary to change our relation part quote character from a single quote to double.

@mikaelene I tested this on a local project and it works without issue. ready for for your further review/approval.

mikaelene

This seems sane. But It is a lot of code to review. I get a little to comfortable with the CI tests sometimes. I think I introduced a bug in snapshots. #108 . So I will review this when I have time.

mikaelene · 2021-02-15T09:04:28Z

dbt/adapters/sqlserver/relation.py

+class SQLServerRelation(BaseRelation):
+    quote_policy: SQLServerQuotePolicy = SQLServerQuotePolicy()
+    include_policy: SQLServerIncludePolicy = SQLServerIncludePolicy()
+    quote_character: str = '"'


I am not a guru in quoting :-). Did we have ' before?

panasenco · 2021-03-01T18:59:50Z

dbt/include/sqlserver/macros/adapters.sql

-    WHERE name='{{ from_relation.schema }}_{{ from_relation.identifier }}_cci' and object_id = OBJECT_ID('{{ from_relation.schema }}.{{ to_relation.identifier }}'))
-    EXEC sp_rename N'{{ from_relation.schema }}.{{ to_relation.identifier }}.{{ from_relation.schema }}_{{ from_relation.identifier }}_cci', N'{{ from_relation.schema }}_{{ to_relation.identifier }}_cci', N'INDEX'
+    WHERE name='{{ from_relation.schema }}_{{ from_relation.identifier }}_cci' and object_id = OBJECT_ID('{{ from_relation }}'))
+    EXEC sp_rename N'{{ from_relation }}.{{ from_relation.schema }}_{{ from_relation.identifier }}_cci', N'{{ from_relation.schema }}_{{ to_relation.identifier }}_cci', N'INDEX'


Why are we prefixing with the entire relation now? That includes double-quoted schema and relation names, which is creating really bizarre output in my run:

EXEC sp_rename N'"dbo"."APPT_COMPLETED_MONTHLY__dbt_tmp".dbo_APPT_COMPLETED_MONTHLY__dbt_tmp_cci', N'dbo_APPT_COMPLETED_MONTHLY_cci', N'INDEX'

panasenco · 2021-03-01T21:22:10Z

@swanderz I've spent several hours today trying to reimplement database switching (see PR #111) in this branch. However, I've run into a wall with the function adapter.drop_relation. The macro create_table_as does the following steps:

Drop the temporary view if it exists
Create the temporary view
Drop the temporary table if it exists
Insert temporary view contents into the temporary table
Drop the temporary view again

Because the temporary view is dropped twice, something screwy happens with the relation cache and the second drop view statement doesn't happen, leaving the temporary view in the database. I'm not certain what exactly is happening because honestly the relation cache is difficult to understand.

My thought is to just drop all uses of adapter.drop_relation and to do it with straightforward SQL rather than trying to understand dbt's relation cache. I don't see the point of the cache anyway, it shaves off milliseconds at most... But I want to check in with you before I do that and ask whether that's a direction you're willing to go.

Is it OK to replace all uses of adapter.drop_relation with some straightforward macro?

dataders · 2021-03-03T09:03:44Z

@panasenco -- I very much appreciate you taking the time to try and make your PR with my refactor. TIL, I had no idea about the relation cache.

Totally open to the "straightforward SQL" approach, but any chance you could open up a different PR with the code you were stuck on so we can look at the error you were seeing? If anything, for curiosity's sake.

dataders added 2 commits January 26, 2021 11:37

auto-render relation names without database

200ada7

fix import path

139807e

dataders requested a review from jtcohen6 January 27, 2021 02:52

jtcohen6 reviewed Jan 27, 2021

View reviewed changes

dbt/adapters/sqlserver/__init__.py Outdated Show resolved Hide resolved

dataders and others added 3 commits January 27, 2021 14:32

need correct import order

a2b820e

Co-authored-by: Jeremy Cohen <[email protected]>

Merge branch 'master' of https://github.com/dbt-msft/dbt-sqlserver in…

2dae934

…to relation-construction

to make error clearer

7dac89f

dataders added 3 commits February 10, 2021 16:49

typo

7dacfca

changes that will make it into next release

3b785e4

throw back to #34

1f952c7

jtcohen6 reviewed Feb 11, 2021

View reviewed changes

dataders added 2 commits February 11, 2021 08:53

will dispatch to sqlserver__ version anyway

25a4253

Merge branch 'posssibly_drop_incremental' of https://github.com/dbt-m…

56d4969

…sft/dbt-sqlserver into relation-construction

dataders added 3 commits February 13, 2021 20:59

safer bet for quoting

46d8a02

re-enable tests

18903b4

do statement "do something"

ddd0b2a

dataders force-pushed the relation-construction branch from 61f7a87 to ddd0b2a Compare February 14, 2021 05:32

dataders added 3 commits February 13, 2021 22:25

not needed

fe7514a

EXEC is needed after all

939e7f0

typo

4d077c7

dataders force-pushed the relation-construction branch 2 times, most recently from 0427207 to 18903b4 Compare February 14, 2021 06:40

TSQL does not require parenthesizing the SELECT

1f11488

dataders changed the title ~~auto-render relation names without database~~ clean up relation usage and default behavior Feb 14, 2021

document change

b49725f

dataders marked this pull request as ready for review February 14, 2021 07:01

dataders requested a review from mikaelene February 14, 2021 19:48

dataders commented Feb 15, 2021

View reviewed changes

mikaelene reviewed Feb 15, 2021

View reviewed changes

dataders requested a review from jtcohen6 February 15, 2021 10:09

dataders added 5 commits February 15, 2021 22:35

leave quoting as is

1d5ca8b

Merge branch 'master' of https://github.com/dbt-msft/dbt-sqlserver in…

83c9b6b

…to relation-construction

simpler reference

93202ce

clarify operation; minimize code

b392d71

fix indentation

8d5f219

dataders force-pushed the relation-construction branch from 5267d23 to 8d5f219 Compare February 17, 2021 09:51

dataders requested a review from mikaelene February 17, 2021 09:58

mikaelene reviewed Feb 19, 2021

View reviewed changes

dataders mentioned this pull request Feb 19, 2021

Adding USE [{{relation.database}}] where appropriate in adapters.sql #111

Closed

panasenco reviewed Mar 1, 2021

View reviewed changes

sdebruyn added chore enhancement New feature or request labels May 22, 2022

dataders closed this Oct 7, 2022

sdebruyn deleted the relation-construction branch May 17, 2023 18:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up relation usage and default behavior #92

clean up relation usage and default behavior #92

dataders commented Jan 26, 2021 •

edited

Loading

dataders commented Jan 26, 2021

dataders commented Feb 11, 2021 •

edited

Loading

jtcohen6 left a comment

jtcohen6 Feb 11, 2021

dataders Feb 11, 2021

dataders commented Feb 11, 2021

dataders Feb 15, 2021

mikaelene Feb 15, 2021

dataders Feb 19, 2021

panasenco Mar 1, 2021

mikaelene left a comment

dataders commented Feb 15, 2021

jtcohen6 commented Feb 15, 2021 •

edited

Loading

dataders commented Feb 17, 2021

mikaelene left a comment

mikaelene Feb 15, 2021

panasenco Mar 1, 2021

panasenco commented Mar 1, 2021

dataders commented Mar 3, 2021

clean up relation usage and default behavior #92

clean up relation usage and default behavior #92

Conversation

dataders commented Jan 26, 2021 • edited Loading

changes

dataders commented Jan 26, 2021

dataders commented Feb 11, 2021 • edited Loading

issue

this jinja-sql

compiles to this

desired behavior

jtcohen6 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dataders commented Feb 11, 2021

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

mikaelene left a comment

Choose a reason for hiding this comment

dataders commented Feb 15, 2021

jtcohen6 commented Feb 15, 2021 • edited Loading

dataders commented Feb 17, 2021

mikaelene left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panasenco commented Mar 1, 2021

dataders commented Mar 3, 2021

dataders commented Jan 26, 2021 •

edited

Loading

dataders commented Feb 11, 2021 •

edited

Loading

jtcohen6 commented Feb 15, 2021 •

edited

Loading