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

Tests for getAs() for Double, Integer types #375

Merged
merged 3 commits into from
Jul 7, 2022

Conversation

navinrathore
Copy link
Contributor

sample input/output
#355 (comment)

@navinrathore
Copy link
Contributor Author

@navinrathore navinrathore marked this pull request as ready for review July 5, 2022 06:22
@navinrathore
Copy link
Contributor Author

navinrathore commented Jul 5, 2022

Null can be cast to any type without any exception. reference

}

/*test values: 5 gbp/gbp/<blank> etc. */
@Test
Copy link
Member

Choose a reason for hiding this comment

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

what happens in these tests with the wrong values?

Copy link
Contributor Author

@navinrathore navinrathore Jul 6, 2022

Choose a reason for hiding this comment

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

They are converted to null. at present schema has "Nullable" field set to true
But is converted to 0.0, when this field is set to false.

Copy link
Member

Choose a reason for hiding this comment

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

ok. so we should move the df creation here and add that test - nullable change. this will be good documentation when we later encounter issues with user data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note
Nullable 'true' in schema. Unsupported values change to null

+-----------+------------+
|fieldDouble|fieldInteger|
+-----------+------------+
|       0.55|          55|
|      1.234|        1234|
|       34.0|        null|
|      99.56|        9956|
|       null|          56|
|       23.0|        null|
|       null|          45|
|       65.0|        null|
|       null|        null|
|       null|          23|
|       56.0|          56|
|       null|        null|
|       null|          34|
|       78.0|        null|
|       78.0|          87|
+-----------+------------+

Nullable 'false' in schema. Unsupported values change to Zero(0.0/0)

+-----------+------------+
|fieldDouble|fieldInteger|
+-----------+------------+
|       0.55|          55|
|      1.234|        1234|
|       34.0|           0|
|      99.56|        9956|
|        0.0|          56|
|       23.0|           0|
|        0.0|          45|
|       65.0|           0|
|        0.0|           0|
|        0.0|          23|
|       56.0|          56|
|        0.0|           0|
|        0.0|          34|
|       78.0|           0|
|       78.0|          87|
+-----------+------------+

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the testcases includes Nullable - true and false for IntegerType and DoubleType.


Dataset<Row> df;

@BeforeEach
Copy link
Member

Choose a reason for hiding this comment

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

why not use @before ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to @BeforeAll

Copy link
Member

@sonalgoyal sonalgoyal left a comment

Choose a reason for hiding this comment

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

Comment for next time, it would be simpler to set up different dfs for different types - doubles/integers rather than having one df. tests should be as atomic as possible.

@sonalgoyal sonalgoyal merged commit 3cb382f into zinggAI:main Jul 7, 2022
@navinrathore navinrathore deleted the TestNullTypes branch July 19, 2022 08:13
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