-
Notifications
You must be signed in to change notification settings - Fork 426
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
fix: parse dynamic table query from DDL #2438
fix: parse dynamic table query from DDL #2438
Conversation
Hey @sonya. Thanks for the PR. This week is pretty tough for us, I will check the PR next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @sonya. Thanks for the contribution.
I have left one comment. I am still not convinced with this change (and the extractors logic in general) and we may still want to remove it and change the behavior with V1 (#2329 (comment)). I guess we can leave it for now (after you correct this according to my comment) and get back to it with the resources redesign which is upcoming.
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.
The "correct" place to specify the query comment in a dynamic table DDL is at the end of the metadata argument list, right before the query. However, when retrieving an existing query from Snowflake using SHOW DYNAMIC TABLE, the comment is placed at the beginning of the metadata argument list. This commit updates the DT query parser to handle comments that can show up in either position.
1af4121
to
615f7be
Compare
Updated the parser to handle a comment as both the first and last parameter before the query |
Thanks, I will check it today! |
/ok-to-test sha=615f7be1379b0dfcc142f0e7d247c7c4067ca9d7 |
Integration tests failure for 615f7be1379b0dfcc142f0e7d247c7c4067ca9d7 |
/ok-to-test sha=8bbf308d490f47359e4660f9a57b5c5a0e778769 |
Integration tests failure for 8bbf308d490f47359e4660f9a57b5c5a0e778769 |
@sonya I have merged the PR, thank you for the contribution! It will be a part of 0.86.0 release. |
🤖 I have created a release *beep* *boop* --- # Release notes [0.86.0](v0.85.0...v0.86.0) (2024-02-15) ## 🎉 **What's new** * add refresh_mode and initialize to dynamic tables ([#2437](#2437)) ([d301b20](d301b20)) * add resource snowflake_user_password_policy_attachment ([#2162](#2162)) ([#2307](#2307)) ([93af462](93af462)) * create a workaround for granting privileges on all pipes ([#2477](#2477)) ([64f2346](64f2346)) * Handle IMPORTED PRIVILEGES privileges in privilege-to-role granting resources ([#2471](#2471)) ([eb20051](eb20051)) * use external functions ([#2454](#2454)) ([417d473](417d473)) * use funcs from sdk ([#2462](#2462)) ([a5f969c](a5f969c)) * use sdk for procedures ([#2450](#2450)) ([94ac78a](94ac78a)) * Use sdk in table constraint resource ([#2466](#2466)) ([d685603](d685603)) * Use tables from SDK ([#2453](#2453)) ([fdb4f88](fdb4f88)) ## 🔧 **Misc** * Add migration notes to the docs and change jira integration ([#2497](#2497)) ([b17f1af](b17f1af)) * Change email and issue reporter ([#2470](#2470)) ([5865655](5865655)) * Grants migration guide ([#2455](#2455)) ([62c70fd](62c70fd)) * Remove unused old implementation from snowflake pkg ([#2458](#2458)) ([2d0e508](2d0e508)) * update password policy attachment ([#2485](#2485)) ([6ec9ff7](6ec9ff7)) ## 🐛 **Bug fixes** * allow DT warehouse to be updated in-place ([#2439](#2439)) ([d565af1](d565af1)) * correct test dependencies ([#2493](#2493)) ([dfb247f](dfb247f)) * FileFormat not detecting changes correctly ([#2436](#2436)) ([018bb74](018bb74)) * Fix few smaller issues ([#2507](#2507)) ([a836871](a836871)) * Fix functions and small other fixes ([#2503](#2503)) ([0d4aba4](0d4aba4)), closes [#2490](#2490) * Fix tag tests in view and in materialized view ([#2457](#2457)) ([2de942a](2de942a)) * Fix task related issues ([#2479](#2479)) ([0385650](0385650)) * Fix tests that base on default data retention ([#2465](#2465)) ([682e28c](682e28c)) * grant privileges to share test terraform dependencies ([#2473](#2473)) ([ede8d95](ede8d95)) * parameter issues ([#2463](#2463)) ([7ee4986](7ee4986)) * parse dynamic table query from DDL ([#2438](#2438)) ([d76815c](d76815c)) * Remove title and body temporarily from jira integration ([#2499](#2499)) ([672c97d](672c97d)) * SHOW GRANTS mapping for share data type ([#2508](#2508)) ([feb4d44](feb4d44)) * user error handling ([#2486](#2486)) ([dfa52b2](dfa52b2)) --- 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>
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
AS
in the name and were affected by spurious diffs with v0.78.0 (as observed in Dynamic tables are not parsing the query correctly and recreate resources on every apply #2329). All of our dynamic tables are properly preserved with code change in this PR.References
Original issue: #2329
Previous attempt to fix: #2421