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

feat: adding secure views for snowflake #3212

Closed
wants to merge 2 commits into from

Conversation

benfdking
Copy link
Contributor

@benfdking benfdking commented Oct 2, 2024

Related to #3070

NOTE I know I need to revert a number of unnecessary format changes.

A few thoughts:

  • I don't think this is tested enough because I haven't run anything against Snowflake
  • I am not certain about the way I create the views, it's a bit ugly
  • More generally, I am slightly nervous about the approach of just adding properties to the root model configuration that are specific to individual engines, as it feels like a way to get way too many confusing configuration points

@tobymao
Copy link
Contributor

tobymao commented Oct 2, 2024

@benfdking is it possible to make this more generic? we've also had a request for transient tabels, i wonder if we can make it easy to specify any type of create table kind

- some open questions about materialized view
@benfdking
Copy link
Contributor Author

I believe this approach is more versatile and can be applied to both TABLE and VIEW types. However, there is a conflict between the new creatable_properties and materialize properties in the case of Snowflake view. This is because, the properties control the same part of the query and according to the Snowflake documentation, the SECURE property has to come first.

The issue can be easily resolved by checking the properties and moving SECURE to the beginning if it is present but I was wondering if we should use this as an opportunity to remove the materialize property?

@tobymao
Copy link
Contributor

tobymao commented Oct 14, 2024

@benfdking what would removing it entail?

@benfdking benfdking closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants