SNOW-1043123: Add greatest/least support to local testing #1247

sfc-gh-jrose · 2024-02-12T20:39:07Z

Please answer these questions before submitting your pull requests. Thanks!

What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

Fixes #1043123
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
- I am adding new logging messages
- I am adding a new telemetry message
- I am adding new credentials
- I am adding a new dependency
Please describe how your code solves the related issue.

This commit adds support for greatest and least in local testing. There are also a bunch of format changes that pre-commit made as well.

sfc-gh-stan · 2024-02-14T00:29:51Z

src/snowflake/snowpark/mock/_functions.py

+
+
+@patch("least")
+def mock_least(*exprs: ColumnEmulator):


The first argument determines the return type: If the first type is numeric, then the return type is ‘widened’ according to the numeric types in the list of all arguments. If the first type is not numeric, then all other arguments must be convertible to the first type. If any of the argument values is NULL, the result is NULL.

from https://docs.snowflake.com/en/sql-reference/functions/least#usage-notes

The first two cases since those require coercion (implicit type conversion, "123" can be implicitly evaluated to 123) for this PR, we should leave a TODO in the docstring.

Does the current implementation support the third case?

I've modified logic and added test cases to show that coercion works correctly.

Sorry I had a typo in the comment above, I meant that coercion is not supported in Local Testing in general so we could ignore the first two cases in this PR and simply leave a TODO in the docstring (including a JIRA ticket)

sfc-gh-stan · 2024-02-14T00:30:37Z

src/snowflake/snowpark/mock/_functions.py

+
+
+@patch("greatest")
+def mock_greatest(*exprs: ColumnEmulator):


Same as below.

sfc-gh-stan

Please add a TODO comment for type coercion in the docstrings and check the edge case when the input column (only) has null values.

sfc-gh-aling · 2024-02-21T21:56:57Z

src/snowflake/snowpark/mock/_functions.py

+def _compare(x: Any, y: Any) -> Tuple[Any, Any]:
+    """
+    Compares two values based on the rules described for greatest/least
+    https://docs.snowflake.com/en/sql-reference/functions/least#usage-notes
+    """
+    if x is None or y is None:
+        return (None, None)
+
+    _x = x if isinstance(x, Number) else float(x)
+    _y = y if isinstance(y, Number) else float(y)
+
+    if _x > _y:
+        return (_x, _y)
+    else:
+        return (_y, _x)


do we consider non-numeric types here?
I see in the description https://docs.snowflake.com/en/sql-reference/functions/least:

LEAST supports all data types, including VARIANT.

I can add some more test cases to be sure. From what I can tell anything non numeric gets cast to a numeric type.

interesting, if I compare a string "abc" with a date "2024-01-01", both would be cast to numeric first?

That case actually generates an exception:

SELECT greatest("A", "B") FROM ( SELECT "A", to_timestamp("B") AS "B" FROM ( SELECT $1 AS "A", $2 AS "B" FROM VALUES ('abc' :: STRING, '2024-01-01 00:00:00' :: STRING)));

Timestamp 'abc' is not recognized

Although if they are all strings it does look like they aren't converted to numerics so the _compare function is incorrect. I'll fix it up.

thanks! do you know what exception class it's raising?
I'm thinking of raising the same error to align the error experience in live mode.
for example, I noticed that in the code type(_x)(y), this might raise ValueError, I'm thinking if we should wrap this into a snowpark client exception

sfc-gh-stan · 2024-02-21T23:24:37Z

src/snowflake/snowpark/mock/_functions.py

@@ -933,6 +933,45 @@ def mock_to_variant(expr: ColumnEmulator):
    return res


+def _compare(x: Any, y: Any) -> Tuple[Any, Any]:


Let's use TypeVar here to indicate that input and output needs to be the same type.

sfc-gh-aling

I have left a comment about the error experience, otherwise the change LGTM

CHANGELOG.md

sfc-gh-aling · 2024-03-01T21:49:49Z

src/snowflake/snowpark/mock/_functions.py

+def _compare(x: Any, y: Any) -> Tuple[Any, Any]:
+    """
+    Compares two values based on the rules described for greatest/least
+    https://docs.snowflake.com/en/sql-reference/functions/least#usage-notes
+    """
+    if x is None or y is None:
+        return (None, None)
+
+    _x = x if isinstance(x, Number) else float(x)
+    _y = y if isinstance(y, Number) else float(y)
+
+    if _x > _y:
+        return (_x, _y)
+    else:
+        return (_y, _x)


thanks! do you know what exception class it's raising?
I'm thinking of raising the same error to align the error experience in live mode.
for example, I noticed that in the code type(_x)(y), this might raise ValueError, I'm thinking if we should wrap this into a snowpark client exception

sfc-gh-aling

LGTM, @sfc-gh-jrose can you help create a JIRA to track the error experience?

SNOW-1043123: Add greatest/least support to local testing

3f82b91

sfc-gh-jrose marked this pull request as ready for review February 13, 2024 16:59

sfc-gh-jrose requested a review from a team as a code owner February 13, 2024 16:59

sfc-gh-jrose requested review from sfc-gh-yixie and sfc-gh-sfan February 13, 2024 16:59

sfc-gh-stan reviewed Feb 14, 2024

View reviewed changes

src/snowflake/snowpark/mock/_functions.py

@patch("greatest")

def mock_greatest(*exprs: ColumnEmulator):

Copy link

Collaborator

sfc-gh-stan Feb 14, 2024

Choose a reason for hiding this comment

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

Same as below.

sfc-gh-stan requested changes Feb 14, 2024

View reviewed changes

Merge branch 'main' into jrose_snow_1043123_greatest_least_support

67ed32d

sfc-gh-jrose requested a review from a team as a code owner February 21, 2024 18:11

sfc-gh-jrose requested a review from sfc-gh-yuwang February 21, 2024 18:11

sfc-gh-jrose added 3 commits February 21, 2024 10:45

Review Feedback

aae18d0

One more test

ee6515c

Forgot a couple cases

67da27d

sfc-gh-aling reviewed Feb 21, 2024

View reviewed changes

A little closer to parity

2180d46

sfc-gh-stan reviewed Feb 21, 2024

View reviewed changes

sfc-gh-jrose and others added 3 commits February 22, 2024 10:31

Review Feedback

1f208dc

a couple more types

974da8f

Merge branch 'main' into jrose_snow_1043123_greatest_least_support

950f2f8

sfc-gh-aling reviewed Mar 1, 2024

View reviewed changes

sfc-gh-jrose added 3 commits March 4, 2024 13:14

Update CHANGELOG

8f62d88

Merge branch 'main' into jrose_snow_1043123_greatest_least_support

5b5bcaf

review feedback

147819e

sfc-gh-stan approved these changes Mar 6, 2024

View reviewed changes

sfc-gh-aling approved these changes Mar 7, 2024

View reviewed changes

Merge branch 'main' into jrose_snow_1043123_greatest_least_support

ca2aa14

sfc-gh-jrose enabled auto-merge (squash) March 7, 2024 00:52

Merge branch 'main' into jrose_snow_1043123_greatest_least_support

84d10cb

sfc-gh-jrose merged commit 3ce2356 into main Mar 7, 2024
55 of 57 checks passed

sfc-gh-jrose deleted the jrose_snow_1043123_greatest_least_support branch March 7, 2024 20:04

github-actions bot locked and limited conversation to collaborators Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1043123: Add greatest/least support to local testing #1247

SNOW-1043123: Add greatest/least support to local testing #1247

sfc-gh-jrose commented Feb 12, 2024

sfc-gh-stan Feb 14, 2024

sfc-gh-jrose Feb 21, 2024

sfc-gh-stan Feb 21, 2024

sfc-gh-stan Feb 14, 2024

sfc-gh-stan left a comment

sfc-gh-aling Feb 21, 2024

sfc-gh-jrose Feb 21, 2024

sfc-gh-aling Feb 21, 2024 •

edited

Loading

sfc-gh-jrose Feb 21, 2024

sfc-gh-jrose Feb 21, 2024 •

edited

Loading

sfc-gh-aling Mar 1, 2024

sfc-gh-stan Feb 21, 2024

sfc-gh-aling left a comment

sfc-gh-aling Mar 1, 2024

sfc-gh-aling left a comment



		@patch("greatest")
		def mock_greatest(*exprs: ColumnEmulator):

		@@ -933,6 +933,45 @@ def mock_to_variant(expr: ColumnEmulator):
		return res


		def _compare(x: Any, y: Any) -> Tuple[Any, Any]:

SNOW-1043123: Add greatest/least support to local testing #1247

SNOW-1043123: Add greatest/least support to local testing #1247

Conversation

sfc-gh-jrose commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-stan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-aling Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-jrose Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-aling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-aling left a comment

Choose a reason for hiding this comment

sfc-gh-aling Feb 21, 2024 •

edited

Loading

sfc-gh-jrose Feb 21, 2024 •

edited

Loading