feat: add refresh_mode and initialize to dynamic tables #2437

sonya · 2024-01-27T21:51:37Z

This PR contains two commits that add refresh_mode and initialize parameters to the snowflake_dynamic_table resource.

REFRESH_MODE and INITIALIZE are parameters that can only be set at creation time. (I believe these were both added within the last couple months.) They are listed under required parameters in the documentation, but both have defaults. There are use cases where, for performance or other reasons, users need to be able to override the defaults.

This PR adds support for these two options by changing refresh_mode from a read-only parameter to a settable optional parameter, and adding initialize as a new parameter. Please see the individual commit messages for additional caveats and implementation notes.

I am opening this PR because we have ongoing development that is blocked without the ability to set these parameters in Terraform. I realize this repo is in the middle of a major framework update, so please let me know if there's anything I can do to help make these features available without being disruptive to the framework work.

Test Plan

acceptance tests
testing with our existing stack. We have a stack that includes ~100 dynamic tables, and I have been running terraform apply with this code change in place. This should provide decent coverage of use cases.

sfc-gh-asawicki · 2024-01-29T16:51:31Z

Hey @sonya. Thanks for the PR. This week is pretty tough for us, I will check the PR next week.

sfc-gh-asawicki

Hey @sonya. Thank you for the contribution one more time! Sorry for the delay in the review.

I left a few comments, please let me know if you have time to adjust the implementation to our requests. @sfc-gh-jcieslak please add more comments if I left something out.

pkg/resources/dynamic_table.go

sfc-gh-asawicki · 2024-02-08T10:17:28Z

pkg/sdk/dynamic_table.go

@@ -22,6 +22,8 @@ type createDynamicTableOptions struct {
 	dynamicTable bool                    `ddl:"static" sql:"DYNAMIC TABLE"`
 	name         SchemaObjectIdentifier  `ddl:"identifier"`
 	targetLag    TargetLag               `ddl:"parameter,no_quotes" sql:"TARGET_LAG"`
+	Initialize   *string                 `ddl:"parameter,no_quotes" sql:"INITIALIZE"`


Please introduce types for RefreshMode and Initialize (e.g. like ColumnConstraintType).

Then you can have the AllRefreshModes list (like AllColumnConstraintTypes) which can be used for validation (ValidateFunc) on the resource parameter (like in table_constraint.go)

Will do - I actually created these types in my first iteration of working on this but removed them after I felt like they didn't actually add value or readability to the code. But I also don't know yall's conventions and figured you'd let me know in the PR review.

Thanks for adding them. Please use them in multiple places:

in createDynamicTableOptions (instead of *string for Initialize and RefreshMode)

in acceptance tests (instead of hardcoded string values)

in resource implementation (instead of hardcoded string values)

in integration test in the SDK

pkg/resources/dynamic_table_acceptance_test.go

pkg/resources/dynamic_table.go

pkg/sdk/testint/dynamic_table_integration_test.go

pkg/resources/dynamic_table.go

sonya · 2024-02-12T05:55:56Z

Thanks for all the pointers and your patience with my code. I think I addressed all the points (but I'm not actually able to run the integration tests because they appear to require an enterprise account. The acceptance tests I can run).

In order to test this on my actual stack, I had to revert the latest commit (#a5f969c) which otherwise causes a panic with an array-out-of-bounds error when parsing existing functions. I'll open a new issue about this.

sfc-gh-asawicki · 2024-02-12T10:42:02Z

Thanks for the changes, I will re-review the PR today.

sfc-gh-jcieslak

Looks good for me 👍 Thanks again for the contribution. Still, let's wait for the @sfc-gh-asawicki re-review.

pkg/resources/dynamic_table_acceptance_test.go

sfc-gh-asawicki · 2024-02-12T16:13:56Z

Hey @sonya. The change is fine, I have just cosmetic remarks. The only one required for me is to use the introduced types in every place instead of using the hardcoded values (i.e. this comment: #2437 (comment)). After that, we will gladly merge the change :) Thank you for all the contributions one more time. :)

Snowflake recently added the ability to set REFRESH_MODE manually when creating dynamic tables. Generally this is for use cases where the user wants to force the DT to use a FULL refresh when the automatic behavior would otherwise be INCREMENTAL. Some reasons users may want to override INCREMENTAL in favor of FULL include a) performance, and b) some edge cases where DTs fail to refresh unless they are forced to use FULL. This commit updates the terraform dynamic table resource to allow users to set `refresh_mode` in configuration. This value can only be set when the DT is created, and forces replacement if the value changes. We suppress most diffs for this parameter because Snowflake also allows users to specify `AUTO` as an input value for refresh_mode, which should be ignored on refresh.

Dynamic tables have an INITIALIZE parameter that can be specified on table creation. This parameter determines when the first refresh of the dynamic table occurs. This option defaults to ON_CREATE, which causes the dynamic table to execute a refresh immediately upon creation. There are cases where this behavior is not desirable. For example, if refreshing takes a long time, we may not want terraform to wait. There are also some cases where a refresh ON_CREATE fails due to time travel issues. In these cases, setting the initialize option to ON_SCHEDULE may be the solution for these issues. This value does not have its own column in SHOW DYNAMIC TABLES and can only be parsed from DDL. This commit also changes the data_timestamp field to a nullable type. This is necessary because when dynamic tables are set to INITIALIZE = ON_SCHEDULE, the dynamic table's data_timestamp is null at creation time, and is only populated when the table refreshes.

* Add type and validations for DynamicTableInitialize options * Update type and add validations for DynamicTableRefreshMode options * Set default values for both initialize and refresh_mode * No longer use DiffSuppressFunc since the default values should match when not specified * Add separate test for initialize and refresh_mode in dynamic table integration test * Add additional steps in dynamic table acceptance test to show what happends when initialize and/or refresh_mode is changed after the dynamic table is already created. * To support the above, read the created_on date from existing dynamic tables as a way to test whether a step has replaced the dynamic table

* Use constants in all places where RefreshMode and Initialize are passed around as arguments * Remove checks for unchanged values between acceptance test steps * Update docs to include the AUTO option for refresh_mode

sonya · 2024-02-14T02:47:31Z

Updated again, thanks for all your time and detailed comments @sfc-gh-asawicki!

sfc-gh-asawicki · 2024-02-14T10:12:46Z

/ok-to-test sha=ef96b3bc5d62b4c0ae0ede9a091bd23dc61a6913

github-actions · 2024-02-14T11:01:26Z

Integration tests failure for ef96b3bc5d62b4c0ae0ede9a091bd23dc61a6913

sfc-gh-asawicki · 2024-02-14T11:08:32Z

I have two minor issues, but I will merge the change and solve them with one of my PRs today.

For future reference, please run make pre-push before pushing. There are multiple checks there, one of which is doc generation (because you changed the description of the arguments, and the docs are generated from it, so there's a diff there - I forgot to mention it earlier).

Thank you for your other contribution, @sonya; we really appreciate your help in improving the provider!

🤖 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>

sfc-gh-asawicki requested changes Feb 8, 2024

View reviewed changes

sonya force-pushed the dynamic-table-refresh-and-initialize branch from 6f7bedd to 9481bff Compare February 12, 2024 05:45

sonya mentioned this pull request Feb 12, 2024

fix: allow DT warehouse to be updated in-place #2439

Merged

1 task

sfc-gh-asawicki self-requested a review February 12, 2024 13:12

sfc-gh-jcieslak previously approved these changes Feb 12, 2024

View reviewed changes

sfc-gh-asawicki reviewed Feb 12, 2024

View reviewed changes

pkg/resources/dynamic_table_acceptance_test.go Outdated Show resolved Hide resolved

sonya added 3 commits February 13, 2024 21:13

sonya dismissed sfc-gh-jcieslak’s stale review via 4ef57ca February 14, 2024 02:14

sonya force-pushed the dynamic-table-refresh-and-initialize branch from 9481bff to 4ef57ca Compare February 14, 2024 02:14

Apply PR feedback on DT initialize/refreshMode

ef96b3b

* Use constants in all places where RefreshMode and Initialize are passed around as arguments * Remove checks for unchanged values between acceptance test steps * Update docs to include the AUTO option for refresh_mode

sonya force-pushed the dynamic-table-refresh-and-initialize branch from 4ef57ca to ef96b3b Compare February 14, 2024 02:21

sfc-gh-asawicki self-requested a review February 14, 2024 11:08

sfc-gh-asawicki approved these changes Feb 14, 2024

View reviewed changes

sfc-gh-jcieslak approved these changes Feb 14, 2024

View reviewed changes

sfc-gh-asawicki merged commit d301b20 into Snowflake-Labs:main Feb 14, 2024
6 of 8 checks passed

snowflake-release-please bot mentioned this pull request Feb 14, 2024

chore(main): release 0.86.0 #2456

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add refresh_mode and initialize to dynamic tables #2437

feat: add refresh_mode and initialize to dynamic tables #2437

sonya commented Jan 27, 2024

sfc-gh-asawicki commented Jan 29, 2024

sfc-gh-asawicki left a comment

sfc-gh-asawicki Feb 8, 2024

sonya Feb 11, 2024

sfc-gh-asawicki Feb 12, 2024 •

edited

Loading

sonya commented Feb 12, 2024

sfc-gh-asawicki commented Feb 12, 2024

sfc-gh-jcieslak left a comment

sfc-gh-asawicki commented Feb 12, 2024

sonya commented Feb 14, 2024

sfc-gh-asawicki commented Feb 14, 2024

github-actions bot commented Feb 14, 2024

sfc-gh-asawicki commented Feb 14, 2024

feat: add refresh_mode and initialize to dynamic tables #2437

feat: add refresh_mode and initialize to dynamic tables #2437

Conversation

sonya commented Jan 27, 2024

Test Plan

sfc-gh-asawicki commented Jan 29, 2024

sfc-gh-asawicki left a comment

Choose a reason for hiding this comment

sfc-gh-asawicki Feb 8, 2024

Choose a reason for hiding this comment

sonya Feb 11, 2024

Choose a reason for hiding this comment

sfc-gh-asawicki Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

sonya commented Feb 12, 2024

sfc-gh-asawicki commented Feb 12, 2024

sfc-gh-jcieslak left a comment

Choose a reason for hiding this comment

sfc-gh-asawicki commented Feb 12, 2024

sonya commented Feb 14, 2024

sfc-gh-asawicki commented Feb 14, 2024

github-actions bot commented Feb 14, 2024

sfc-gh-asawicki commented Feb 14, 2024

sfc-gh-asawicki Feb 12, 2024 •

edited

Loading