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

Dynamic tables are not parsing the query correctly and recreate resources on every apply #2329

Closed
dtiesling opened this issue Jan 9, 2024 · 19 comments · Fixed by #2421
Closed
Labels
bug Used to mark issues with provider's incorrect behavior

Comments

@dtiesling
Copy link

Terraform CLI and Provider Versions

terraform v1.3.9
snowflake-provider v0.82.0

Terraform Configuration

resource "snowflake_dynamic_table" "lease_signature" {
  database  = "DB"
  name      = "LEASE_SIGNATURE"
  schema    = "PUBLIC"
  warehouse = "COMPUTE_WH"
  target_lag {
    maximum_duration = "20 minutes"
  }
  query = "SELECT * FROM DB.PUBLIC.LEASE_SIGNATURE"
}

Expected Behavior

I expect the state to show the query as "SELECT * FROM ADDITIONAL_TERMS FROM DB.PUBLIC.FOO"

Actual Behavior

State says query = "_SIGNATURE\" lag = '20 minutes' refresh_mode = 'AUTO' initialize = 'ON_CREATE' warehouse = COMPUTE_WH AS SELECT * FROM DB.PUBLIC.LEASE_SIGNATURE"

Steps to Reproduce

  1. terraform apply

How much impact is this issue causing?

High

Logs

No response

Additional Information

The select query in the example has been altered to not expose our code. If you cannot reproduce please reach out to me and I will help reproduce.

@dtiesling dtiesling added the bug Used to mark issues with provider's incorrect behavior label Jan 9, 2024
@dtiesling
Copy link
Author

I tested terraform v1.6.6 as well. Same behavior.

@sfc-gh-asawicki
Copy link
Collaborator

Hey, @dtiesling. Thanks for reporting the issue.

I can't reproduce it. We even have a test verifying such a simple query:

func TestAcc_DynamicTable_basic(t *testing.T) {
Maybe to reproduce it, we should know the query you are using (because as you say, you altered it not to expose your code)? Is the provided config failing for you with such an altered query?

@dtiesling
Copy link
Author

@sfc-gh-asawicki I emailed you with one of the the exact terraform resources that is getting this behavior.

@sfc-gh-asawicki
Copy link
Collaborator

@dtiesling, thanks; I will try it out and follow up here or through email.

@dtiesling
Copy link
Author

@sfc-gh-asawicki were you able to reproduce my issue?

@sfc-gh-asawicki
Copy link
Collaborator

Hey @dtiesling. I did not manage to check it last week I have it planned for this one, sorry for the delay.

@sonya
Copy link
Contributor

sonya commented Jan 24, 2024

I have been looking into this issue (it's only a mild annoyance for us because the change doesn't actually end up getting applied).

This appears to affect dynamic tables whose names include the string AS (e.g. REASON). I believe the issue is that we're using a simple substring to detect where the query begins

query := strings.TrimSpace(text[strings.Index(text, "AS")+3:])

I was looking into a fix that involves creating a new method on ViewSelectStatementExtractor for dynamic tables. If you wait long enough I may eventually open a PR to try this out, but it's probably not a good idea to wait for me.

@dtiesling
Copy link
Author

I'll test that theory today. Is the fix as simple as changing the trim to use " AS "?

@dtiesling
Copy link
Author

dtiesling commented Jan 24, 2024

@sfc-gh-asawicki @sonya confirmed I'm only seeing the issue on tables that contain "as". Unfortunately that is the majority of our tables.

@sonya
Copy link
Contributor

sonya commented Jan 24, 2024

@dtiesling a workaround for you might be to use an older version of the Snowflake provider. We are using 0.78.0 and while it does show this spurious diff, it doesn't actually get applied. That behavior was changed in #2287

@dtiesling
Copy link
Author

@sonya thanks! If you change the select query does the older version correctly recreate the table?

@sonya
Copy link
Contributor

sonya commented Jan 24, 2024

Ah, that's an unfortunate bug with the old version. If we want to change the query we have to delete and recreate the table

@dtiesling
Copy link
Author

@sfc-gh-asawicki I tossed a draft PR in with a proposed fix. I don't have the bandwidth atm to build/host the provider locally for testing or set up a test snowflake account to run the tests (even unit tests seem to fail without some real credentials?) though.

@sfc-gh-asawicki
Copy link
Collaborator

I will be looking into this issue later today. Thanks for analyzing it further, @sonya @dtiesling! This will be helpful.

@sfc-gh-asawicki
Copy link
Collaborator

@sonya @dtiesling I have merged the changes, please test it after we release the v0.85.0 version.

@sonya
Copy link
Contributor

sonya commented Jan 27, 2024

@sfc-gh-asawicki thanks for the update. Unfortunately the fix doesn't work with a lot of our queries because we make liberal use of CTEs. Those queries start with WITH instead of SELECT.

I think I will actually PR the change I proposed above, as I need to open a few PRs for other DT features anyway. I understand that you guys are in the middle of a big rewrite, so I'm hoping that I won't cause too much disruption with peripheral feature contributions.

@sfc-gh-asawicki
Copy link
Collaborator

@sonya, we are open to contributions. :)

So that you know, we are removing the old logic from the snowflake package. Additionally, during the discussions, we were really close to pulling the read for the query entirely and accepting the downsides of such a move (import not fully functional and external changes not recognized by the provider). We aim to simplify the logic and the usage of the provider (check that part in the explanation comment:

* We did not want to complicate the implementation too much by introducing parsers.
). I can't guarantee that we won't stick with the option to not read with V1 of the provider.

@sfc-gh-asawicki
Copy link
Collaborator

@sonya one more thought: are you starting the queries with something else than AS SELECT or AS WITH?

sfc-gh-jcieslak pushed a commit that referenced this issue Feb 2, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.85.0](v0.84.1...v0.85.0)
(2024-02-01)


### 🎉 **What's new:**

* Add API integration to the SDK
([#2409](#2409))
([23acda5](23acda5))
* add application to sdk
([#2350](#2350))
([de97ad8](de97ad8))
* add external funcs to sdk
([#2440](#2440))
([c8cf09b](c8cf09b))
* Add grant privileges to share resource
([#2447](#2447))
([d8241a5](d8241a5))
* Add materialized view to the SDK
([#2403](#2403))
([a5ce699](a5ce699))
* Add notification integration to the SDK
([#2412](#2412))
([d84240c](d84240c))
* add sequences to sdk
([#2351](#2351))
([d2e5ffd](d2e5ffd))
* add snowflake grant privileges to account role
([#2365](#2365))
([e3d086e](e3d086e))
* add streamlits to sdk
([#2400](#2400))
([129d24c](129d24c))
* add-call-with to sdk
([#2337](#2337))
([ebcd1bc](ebcd1bc))
* stages migration follow-up
([#2372](#2372))
([3939dbe](3939dbe))
* Use API integration from SDK
([#2429](#2429))
([1ccc864](1ccc864))
* Use managed account from the SDK
([#2420](#2420))
([3aaa080](3aaa080))
* Use materialized views and views from SDK
([#2448](#2448))
([dc66d02](dc66d02))
* Use notification integration from sdk
([#2445](#2445))
([e8915cc](e8915cc))
* use roles from the SDK
([#2405](#2405))
([c645b4d](c645b4d))
* Use row access policy from SDK
([#2428](#2428))
([119af5e](119af5e))
* Use SDK in the storage integration
([#2380](#2380))
([ce0741c](ce0741c))
* use sequence from sdk and add ordering attr
([#2419](#2419))
([973b8f7](973b8f7)),
closes
[#2387](#2387)
* Use stage from sdk
([#2427](#2427))
([c17effd](c17effd))


### 🔧 **Misc**

* add missing deprecation message
([#2451](#2451))
([77de569](77de569))


### 🐛 **Bug fixes:**

* account role test
([#2422](#2422))
([c1b47d1](c1b47d1))
* Adjust tests after Snowflake behavior change
([#2404](#2404))
([8c03ffb](8c03ffb))
* app-pkg unset
([#2399](#2399))
([fedb1df](fedb1df))
* Fix some bugs
([#2421](#2421))
([dec7cd9](dec7cd9)),
closes
[#2358](#2358)
[#2369](#2369)
[#2329](#2329)
* snowflake_grant_privileges_to_role read
([#2424](#2424))
([5385cec](5385cec))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: snowflake-release-please[bot] <105954990+snowflake-release-please[bot]@users.noreply.github.com>
sfc-gh-asawicki added a commit that referenced this issue Feb 12, 2024
Previously we have been extracting the `query` portion of dynamic table
definitions from the stored DDL in Snowflake by searching for the string
position of keywords that preceded the query. However, this approach has
proven inadequate as there are many ways to write working dynamic table
definitions that conflict with string position assumptions.
    
Observing that definitions of views and materialized views have faced
similar challenges but are currently working, this commit extends the
existing ViewSelectStatementExtractor that is currently being used to
extract the query definition from view and materialized view DDL
statements, applying the same approach to dynamic table DDL statements.

This is an attempt to address #2329

## Test Plan
* [x] acceptance tests
* [x] testing in our existing stack. We have an existing stack with ~100
dynamic tables where many have CTEs and fail to be parsed with v0.85.0.
A few contains `AS` in the name and were affected by spurious diffs with
v0.78.0 (as observed in #2329). All of our dynamic tables are properly
preserved with code change in this PR.

## References

Original issue:
#2329
Previous attempt to fix:
#2421

---------

Co-authored-by: Artur Sawicki <[email protected]>
@sfc-gh-asawicki
Copy link
Collaborator

Closing the issue, there were fixes in 0.83.0 and 0.85.0 versions. Please create a new issue if the problem persists for the newest version of the provider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Used to mark issues with provider's incorrect behavior
Projects
None yet
3 participants