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

Fix column declaration and enhance regexp_like support for Snowflake #107

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

lawrenceadams
Copy link
Collaborator

This PR adds support for snowflake as a backend through various steps.

  • Correct the declaration of the Id column across multiple seed files
  • Add a new regexp_like macro for Snowflake, and...
  • Refactor the measurement model to utilize this new macro.
  • Enable quoting of columns for Snowflake compatibility.

@lawrenceadams lawrenceadams added the enhancement New feature or request label Dec 11, 2024
Copy link
Collaborator Author

@lawrenceadams lawrenceadams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katy-sadowski this may not be desired but given we've done a lot of cleaning up recently I thought it would be silly to not try and add the finishing touches to enable snowflake support.

Currently missing documentation (thought I'd await people's views first)!

If we decide against it then at the very least take 1a2e6cd changes (took me too long to figure out why it was causing problems 💀)!

Copy link
Collaborator

@katy-sadowski katy-sadowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for adding new DB platforms! I use Snowflake at work, too, so I can help test.

Thanks for these fixes which I'm sure were "fun" to figure out 😜 I have one suggested simplification on the regex bit, lmk what you think.

Comment on lines 59 to 63
, CASE
WHEN o.observation_value ~ '^[-+]?[0-9]+\.?[0-9]*$'
WHEN {{ regexp_like("o.observation_value", "^[-+]?[0-9]+\.?[0-9]*$") }}
THEN {{ dbt.cast("o.observation_value", api.Column.translate_type("decimal")) }}
ELSE {{ dbt.cast("null", api.Column.translate_type("decimal")) }}
END AS value_as_number
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new macro is useful and we should keep it for future use (thanks!!!) - but looking at this gross regex, maybe we could use safe_cast here instead? https://docs.getdbt.com/reference/dbt-jinja-functions/cross-database-macros#safe_cast

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@katy-sadowski Had another look through the cross-DB macros and fairly sure there's no regular expression oriented ones sadly - I don't think there is a way around this without using custom dispatchers / macros like this

It raises the question of do we event want to add ❄️ support! I think it is nice to have - just wish they used the ~ operator...

I looked into transpiring with SQLGlot, but I think thats a lot of work for relatively low yield - pulling out the DAG from dbt etc as per here #39 (comment) is quite a lot of work which isn't in the scope of the project (IMHO!)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahhh sorry i forgot about what you already said in #68 about safe_cast not being supported on all platforms. let's carry on with the regex approach! agree re: sqlglot.

@lawrenceadams
Copy link
Collaborator Author

Honestly hair pulling stupidness!

I'm not sure safe_cast is functionally equivalent but will give it ago! Sadly dbt has no native binding for this function ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants