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

SNOW-802269-Jsontuple-cbrt-fromjson-datesub-trycast #161

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

Conversation

sfc-gh-sjayabalan
Copy link
Collaborator

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

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

Added following functions in java and scala.
a. json_tuple()
b.from_json()
c.date_sub()
d.try_cast()
e.cbrt()

  1. Fill out the following pre-review checklist:

    • [ yes] 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
  2. Please describe how your code solves the related issue.

Added following functions as wrapper to java and scala functions.

Pre-review checklist

(For Snowflake employees)

  • This change has passed precommit
  • I have reviewed code coverage report for my PR in (Sonarqube)

* The column argument must be a string column in Snowflake.
*/
def try_cast(e: Column, targetType: DataType): Column = {
e.cast(targetType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have tests cases to validate that this returns null if cast fails? I think this might fail and that you might need to use try_cast built int

@@ -3186,4 +3188,81 @@ public void to_utc_timestamp() {
Row[] expected = {Row.create(Timestamp.valueOf("2024-04-05 01:02:03.0"))};
checkAnswer(df.select(Functions.to_utc_timestamp(df.col("a"))), expected, false);
}

@Test
public void json_tuple1() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

use a more representative test name like json_tuple_

maybe json_tuple_single_parameter ? for example

}

@Test
public void json_tuple2() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as with preivous

}

@Test
public void try_cast() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

need more test cases. You need to test several combinarions

df = spark.createDataFrame(
[(2, "123"), (5, "Bob"), (3, None)], ["age", "name"])
df.select(df.name.try_cast(LongType())).show()
+----+
|name|
+----+
| 123|
|NULL|
|NULL|
+----+
Structure types should be consider as well:

df.filter(F.array_contains(F.lit('Key'), F.col('KEY_UNSTRCUTURED_ARRAY ').cast('ARRAY(VARCHAR)')).show()

or

df.filter(F.array_contains(F.lit('Key'), F.col('KEY_UNSTRCUTURED_ARRAY ').cast(T.ArraryType(T.StringType()))).show()


@Test
public void date_sub() {
DataFrame df =
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need more tests, what happens with null, what happens with large values


@Test
public void from_json() {
DataFrame df =
Copy link
Collaborator

Choose a reason for hiding this comment

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

positive an negative tests needed.

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.

3 participants