[SPARK-41201][CONNECT][PYTHON] Implement `DataFrame.SelectExpr` in Python client by amaliujia · Pull Request #38723 · apache/spark

amaliujia · 2022-11-19T05:59:57Z

What changes were proposed in this pull request?

Implement DataFrame.SelectExpr in Python client. SelectExpr also has a good amount of usage.

Why are the changes needed?

API coverage.

Does this PR introduce any user-facing change?

NO

How was this patch tested?

UT

amaliujia · 2022-11-19T06:00:05Z

@zhengruifeng @HyukjinKwon

grundprinzip · 2022-11-19T14:03:13Z

python/pyspark/sql/connect/dataframe.py

So this becomes an unresolved attribute and just works out of the box?

Actually it becomes Expression.Builder().setExpressionString() which are SQL expression strings.

str could be different things in DataFrame API.

This was not necessarily the question that I had, but I was not remembering correctly the type interface to Project:

def __init__(self, child: Optional["LogicalPlan"], *columns: "ExpressionOrString") -> None:

In this case SQLExpression is an expression and it just works.

grundprinzip · 2022-11-20T13:17:56Z

python/pyspark/sql/connect/column.py

should we assert here that expr is string and not another expression?

In this implementation, we don't need to because the caller has verified the type (thus mypy does not complain).

I think this is a good question:
Generally speaking, I think for public API, we should throw user-facing exception, for internal API, we can assert when we want to defensive check unexpected input.

So it is a question of if we want to enforce checking cross all public/private API (by corresponding ways). I guess maybe not now but worth it at a right time (maybe before 3.4 release).

I think that's fine. One point of having type hints is to avoid asserts on those types too.

grundprinzip

Just one question.

AmplabJenkins · 2022-11-20T16:58:32Z

Can one of the admins verify this patch?

HyukjinKwon · 2022-11-22T06:48:22Z

python/pyspark/sql/connect/column.py

I still don't like kind of naming .. but this is at least somewhat consistent with what we have in DSv2 so I am fine.

Let's see in the future... I guess we will need to name more...

HyukjinKwon · 2022-11-22T06:49:20Z

python/pyspark/sql/tests/connect/test_connect_basic.py

comment with a JIRA ID?

grundprinzip · 2022-11-22T07:05:57Z

python/pyspark/sql/tests/connect/test_connect_basic.py

Didn't we fix this to grpc.RPCError?

hmmm I guess that was gone during code conflict and resolution then good fix is gone.

…thon client.

HyukjinKwon · 2022-11-23T01:09:21Z

Merged to master.

zhengruifeng · 2022-11-23T01:13:17Z

python/pyspark/sql/tests/connect/test_connect_basic.py

+            .toPandas(),
+        )
+
+    @unittest.skip("test_fill_na is flaky")


@amaliujia why you disable this test again?

ah .. I didn't notice this. Can we enable this back?

I think so, will send a followup for it

I am pretty sure I removed this after conflict resolution.

Actually Martin pointed out another case: #38723 (comment)

Basically it seems happened more than once that after code conflict resolution, the code I want to keep is gone.|

Maybe I should always do a -i commits square to in case more than 1 commit rebase causing unexpected result.

I will follow up this soon.

I am really guessing if I have more than 1 commit locally, if the first one I resolve the conflict, the following commit that might add something back silently.....

no worries, add it back in #38763

### What changes were proposed in this pull request? Reenable test_fill_na ### Why are the changes needed? `test_fill_na` was disabled by mistake in #38723 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? reenabled test Closes #38763 from zhengruifeng/connect_reenable_test_fillna. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

…thon client ### What changes were proposed in this pull request? Implement `DataFrame.SelectExpr` in Python client. `SelectExpr` also has a good amount of usage. ### Why are the changes needed? API coverage. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? UT Closes apache#38723 from amaliujia/support_select_expr_in_python. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

### What changes were proposed in this pull request? Reenable test_fill_na ### Why are the changes needed? `test_fill_na` was disabled by mistake in apache#38723 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? reenabled test Closes apache#38763 from zhengruifeng/connect_reenable_test_fillna. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

…thon client ### What changes were proposed in this pull request? Implement `DataFrame.SelectExpr` in Python client. `SelectExpr` also has a good amount of usage. ### Why are the changes needed? API coverage. ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? UT Closes apache#38723 from amaliujia/support_select_expr_in_python. Authored-by: Rui Wang <rui.wang@databricks.com> Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>

### What changes were proposed in this pull request? Reenable test_fill_na ### Why are the changes needed? `test_fill_na` was disabled by mistake in apache#38723 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? reenabled test Closes apache#38763 from zhengruifeng/connect_reenable_test_fillna. Authored-by: Ruifeng Zheng <ruifengz@apache.org> Signed-off-by: Ruifeng Zheng <ruifengz@apache.org>

github-actions bot added CONNECT CORE PYTHON SQL labels Nov 19, 2022

grundprinzip reviewed Nov 19, 2022

View reviewed changes

grundprinzip reviewed Nov 20, 2022

View reviewed changes

grundprinzip approved these changes Nov 20, 2022

View reviewed changes

HyukjinKwon reviewed Nov 22, 2022

View reviewed changes

HyukjinKwon approved these changes Nov 22, 2022

View reviewed changes

grundprinzip reviewed Nov 22, 2022

View reviewed changes

amaliujia added 2 commits November 22, 2022 16:08

[SPARK-41201][CONNECT][PYTHON] Implement DataFrame.SelectExpr in Py…

d4331a6

…thon client.

update

7802dcc

amaliujia force-pushed the support_select_expr_in_python branch from 3df9929 to 7802dcc Compare November 23, 2022 00:24

HyukjinKwon closed this in 2c8da56 Nov 23, 2022

zhengruifeng reviewed Nov 23, 2022

View reviewed changes

zhengruifeng mentioned this pull request Nov 23, 2022

[SPARK-41201][CONNECT][PYTHON][TEST][FOLLOWUP] Reenable test_fill_na #38763

Closed

Comments

Conversation

amaliujia commented Nov 19, 2022

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

amaliujia commented Nov 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grundprinzip left a comment

Choose a reason for hiding this comment

Uh oh!

AmplabJenkins commented Nov 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Nov 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants