Skip to content

Conversation

fivetran-amrutabhimsenayachit
Copy link
Collaborator

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds test coverage for Snowflake's string concatenation functions (CONCAT and CONCAT_WS) to verify their return type annotations in the optimizer.

Key Changes

  • Added 5 new test cases for CONCAT and CONCAT_WS functions in Snowflake dialect
  • Tests verify that both functions return VARCHAR type regardless of input parameters

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@fivetran-amrutabhimsenayachit fivetran-amrutabhimsenayachit marked this pull request as draft September 5, 2025 17:56
Copy link
Collaborator

@georgesittas georgesittas left a comment

Choose a reason for hiding this comment

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

Given Snowflake's docs say:

The input expressions must all be strings, or all be binary values.

Let's make sure we test the binary values as well.

@fivetran-amrutabhimsenayachit
Copy link
Collaborator Author

Given Snowflake's docs say:

The input expressions must all be strings, or all be binary values.

Let's make sure we test the binary values as well.

The base Dialect class has exp.Concat in its TYPE_TO_EXPRESSIONS[VARCHAR] mapping . Thus, CONCAT always returns VARCHAR, regardless of input types BINARY or NULL values.
We will have to override this logic for both CONCAT and CONCAT_WS methods for snowflake.

@fivetran-amrutabhimsenayachit fivetran-amrutabhimsenayachit marked this pull request as ready for review September 5, 2025 18:29
@georgesittas
Copy link
Collaborator

@fivetran-amrutabhimsenayachit have you created a virtual environment for Python? Seems like there may be a mismatch between the ruff version SQLGlot installs vs what you have locally. Try the following:

$ python -m venv .venv
$ source .venv/bin/activate
$ make install-dev
$ make style

This should fix any formatting issues.

The base Dialect class has exp.Concat in its TYPE_TO_EXPRESSIONS[VARCHAR] mapping . Thus, CONCAT always returns VARCHAR, regardless of input types BINARY or NULL values.
We will have to override this logic for both CONCAT and CONCAT_WS methods for snowflake.

Overriding makes sense, but with a quick look I feel like the current implementations are quite complicated. What about using _annotate_by_arg for both of them?

@fivetran-amrutabhimsenayachit
Copy link
Collaborator Author

have you created a virtual environment for Python
@georgesittas , No, I do not have virtual environment for python. Seems like the new functions added have not been formatted properly. I did not run the make style before commit. Thanks for these commands, will try these.

Comment on lines +1558 to +1560
# dialect: snowflake
REVERSE(NULL);
NULL;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this was added by mistake– I got rid of it in main. Let's git rebase off of it.

@@ -502,6 +502,7 @@ class Snowflake(Dialect):
expr_type: lambda self, e: self._annotate_by_args(e, "this")
for expr_type in (exp.Reverse,)
},
exp.ConcatWs: lambda self, e: self._annotate_concat_ws(e),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe we can simply do:

exp.ConcatWs: lambda self, e: self._annotate_by_args(e, "expressions")

Don't think that we need _annotate_concat_ws. Lemme know if I missed something.

@fivetran-amrutabhimsenayachit fivetran-amrutabhimsenayachit merged commit e667f0e into main Sep 8, 2025
6 checks passed
@fivetran-amrutabhimsenayachit fivetran-amrutabhimsenayachit deleted the update_tests_for_concat_sf branch September 8, 2025 12:42
georgesittas pushed a commit that referenced this pull request Sep 8, 2025
* Resolved Conflicts

* Handle Binary and Null cases

* Fixed ruff formatting

* Revert "Fixed ruff formatting"

This reverts commit bd5c102.

* Addressed review comments

* Addressed comments
georgesittas pushed a commit that referenced this pull request Sep 8, 2025
* feat(singlestore): Added handling of exp.JSONArray

* Update(Snowflake): Update tests for concat string function (#5809)

* Resolved Conflicts

* Handle Binary and Null cases

* Fixed ruff formatting

* Revert "Fixed ruff formatting"

This reverts commit bd5c102.

* Addressed review comments

* Addressed comments

* Fixed merge issues

---------

Co-authored-by: fivetran-amrutaayachit <[email protected]>
georgesittas pushed a commit that referenced this pull request Sep 8, 2025
…MP (#5808)

* feat(singlestore): Added support of UTC_TIMESTAMP and CURRENT_TIMESTAMP

* Fixed BigQuery

* Fixed tests

* Updated test as per comments

* Update(Snowflake): Update tests for concat string function (#5809)

* Resolved Conflicts

* Handle Binary and Null cases

* Fixed ruff formatting

* Revert "Fixed ruff formatting"

This reverts commit bd5c102.

* Addressed review comments

* Addressed comments

* Removed unexisting UTC_TIMESTAMP and UTC_TIME functions from bigquery and added them to Oracle/MySQL

* Removed bigquery changes

---------

Co-authored-by: fivetran-amrutaayachit <[email protected]>
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