-
Notifications
You must be signed in to change notification settings - Fork 10
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: Implements basic snowflake session variables via SET/UNSET #111
feat: Implements basic snowflake session variables via SET/UNSET #111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great feature, thanks for implementing this! Some suggestions (including optional ones) below
tests/test_fakes.py
Outdated
cur.execute('SET var3 = parse_json(\'{"k1": "v1"}\');') | ||
cur.execute("SET var4 = array_construct(1, 2, 3);") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests should pass on a real Snowflake instance to confirm they correctly match Snowflake's behaviour. Try running this on Snowflake. When I do, for these two lines, I get the error:
ProgrammingError: 000002 (0A000): Unsupported feature 'assignment from non-constant source expression'.
Does that happen for you?
This is what works for me:
cur.execute('SET var3 = parse_json(\'{"k1": "v1"}\');') | |
cur.execute("SET var4 = array_construct(1, 2, 3);") | |
cur.execute('SET var3 = (select to_json(parse_json(\'{"k1": "v1"}\')));') | |
cur.execute("SET var4 = (select to_json(array_construct(1, 2, 3)));") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't believe I missed this initially 🤦 .
tests/test_fakes.py
Outdated
cur.execute("SET day = CURRENT_DATE();") | ||
|
||
cur.execute("select $var1, $var2, $var3, $var4, $day;") | ||
assert cur.fetchall() == [(1, "hello", '{"k1":"v1"}', [1, 2, 3], datetime.date.today())] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match Snowflake:
assert cur.fetchall() == [(1, "hello", '{"k1":"v1"}', [1, 2, 3], datetime.date.today())] | |
assert cur.fetchall() == [(1, "hello", '{"k1":"v1"}', '[1,2,3]', datetime.date.today())] |
tests/test_fakes.py
Outdated
try: | ||
cur.execute("select $var3;") | ||
raise AssertionError("Expected exception") | ||
except duckdb.duckdb.InvalidInputException: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test for an exception I'd use pytest.raises, eg:
try: | |
cur.execute("select $var3;") | |
raise AssertionError("Expected exception") | |
except duckdb.duckdb.InvalidInputException: | |
pass | |
with pytest.raises(ProgrammingError, match='Session variable '$VAR3' does not exist'): | |
cur.execute("select $var3;") |
Note also fakesnow should raise the snowflake ProgrammingError
rather than a duckdb error, to avoid the duckdb implementation leaking out into clients.
And optionally have a similar error message to Snowflake eg:
Session variable '$VAR3' does not exist
tests/test_fakes.py
Outdated
# variables are scoped to the session, so they should be available in a new cursor. | ||
with conn.cursor() as cur: | ||
cur.execute("select $var1, $var2, $var4;") | ||
assert cur.fetchall() == [(1, "hello", [1, 2, 3])] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert cur.fetchall() == [(1, "hello", [1, 2, 3])] | |
assert cur.fetchall() == [(1, "hello", '[1,2,3]')] |
fakesnow/fakes.py
Outdated
if Variables.is_variable_modifier(transformed): | ||
self._conn.variables.update_variables(transformed) | ||
transformed = transforms.SUCCESS_NOP # Nothing further to do if its a SET/UNSET operation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(optional) for consistency I think this could be handled like any other transform, eg: in transforms.py
as the method
def update_variables(
expression: exp.Expression,
variables: dict[str, Any],
) -> exp.Expression:
# if is variable modifier, update variables dict and return transforms.SUCCESS_NOP
These comments make sense. I'll work through these changes and notify you when its ready for review again. |
Just trying to test functions can be used so extra complexity of parse_json is unneccessary.
@tekumara I've made updates from all the comments I believe this is ready for review again :). |
Prefect thank you! |
🤖 I have created a release *beep* *boop* --- ## [0.9.19](v0.9.18...v0.9.19) (2024-07-08) ### Features * Implements basic snowflake session variables via SET/UNSET ([#111](#111)) ([7696cbd](7696cbd)) ### Chores * **deps-dev:** bump pyright from 1.1.366 to 1.1.369 ([#112](#112)) ([7656ab9](7656ab9)) --- 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: potatobot-prime[bot] <132267321+potatobot-prime[bot]@users.noreply.github.com>
* main: chore(main): release 0.9.20 (tekumara#115) fix: $$ not considered a variable fix: concurrent connection write-write conflict refactor: extract test_connect.py feat: SHOW PRIMARY KEYS for table (tekumara#114) chore(main): release 0.9.19 (tekumara#113) feat: Implements basic snowflake session variables via SET/UNSET (tekumara#111) chore(deps-dev): bump pyright from 1.1.366 to 1.1.369 (tekumara#112) chore(main): release 0.9.18 (tekumara#110)
Implements the very basic usages of sql variables
as described in https://docs.snowflake.com/en/sql-reference/session-variables.
List of features supported and not supported by this PR: