add unit tests for expression functions#1121
Conversation
|
FYI @deanm0000 |
|
@mesejo @kosiew @chenkovsky @kevinjqliu Would any of you mind doing a review? We haven't released datafusion-python in a while and this is one of the PRs waiting to go in. 3/3 |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Looks good.
I added some suggestions.
python/tests/test_expr.py
Outdated
| def test_expr_functions(ctx, function, expected_result): | ||
| ctx = SessionContext() |
There was a problem hiding this comment.
The second line can be deleted since ctx is available as a fixture
python/tests/test_expr.py
Outdated
| ), | ||
| pytest.param( | ||
| # Valid values of acosh must be >= 1.0 | ||
| functions.round((col("a").abs() + lit(1.0)).abs().acosh(), lit(4)), |
There was a problem hiding this comment.
Second abs() is redundant since lit(1.0)) is already ≥ 0.
This can be simplified to:
functions.round((col("a").abs() + lit(1.0)).acosh(), lit(4))
python/tests/test_expr.py
Outdated
| result = df.collect() | ||
|
|
||
| assert len(result) == 1 | ||
| assert result[0].column(0) == expected_result |
There was a problem hiding this comment.
Directly using assert result[0].column(0) == expected_result can be brittle for NaN or infinite values. Prefer Arrow’s built-in equality check:
assert result[0].column(0).equals(expected_result)
python/tests/test_expr.py
Outdated
| ], | ||
| ) | ||
| def test_expr_functions(ctx, function, expected_result): | ||
| ctx = SessionContext() |
There was a problem hiding this comment.
| ctx = SessionContext() |
python/tests/test_expr.py
Outdated
| result = df.collect() | ||
|
|
||
| assert len(result) == 1 | ||
| assert result[0].column(0) == expected_result |
There was a problem hiding this comment.
| assert result[0].column(0) == expected_result | |
| assert result[0].column(0).equals(expected_result) |
|
Thank you all! Great suggestions. |
5 participants
Which issue does this PR close?
Follow on to #1116
Rationale for this change
We don't have code coverage for these expression functions
What changes are included in this PR?
Unit tests
Are there any user-facing changes?
None