feat(snowflake): transpile Snowflake IDENTIFIER function to DuckDB#7723
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Hey @fivetran-kwoodbeck, I don't think this is the right approach. We should parse the IDENTIFIER(...) argument, and only when it's a string, otherwise you risk breaking case-sensitive identifiers. I wonder if we should just use to_column and to_table directly and produce semantically equivalent SQL without the IDENTIFIER call.
Falling back to an Anonymous node in other cases is good. I would also have a try-except block to make sure we gracefully handle parse failures and fall back to the IDENTIFIER(...) syntax.
Check this out too: https://github.com/fivetran/sqlglot-integration-tests/pull/452#pullrequestreview-4467142494.
12b5a1f to
d97ad27
Compare
d97ad27 to
12e2d66
Compare
12e2d66 to
bdeb4b5
Compare
georgesittas
left a comment
There was a problem hiding this comment.
@fivetran-kwoodbeck this looks better. One comment is pending and I think the submodule pointer needs to be updated as well.
bdeb4b5 to
161022e
Compare
@georgesittas I replied and updated the submodule |
Snowflake's
IDENTIFIER(), which resolves a string to a runtime identifier, previously fell through toexp.Anonymous. This folds it into a typedexp.Identifiernode (flagged withidentifier_func=True) so the Snowflake generator reconstructsIDENTIFIER('name')and the DuckDB generator emits a native identifier. It includes splitting dot-qualified names likeIDENTIFIER('t1.col')into a properexp.Dotchain.Non-foldable args like
IDENTIFIER($var)remain asexp.Anonymousso they round-trip correctly.Behavior