Skip to content

Conversation

@beliefer
Copy link
Contributor

@beliefer beliefer commented Sep 30, 2022

What changes were proposed in this pull request?

We are very concerned about the progress of the Spark Connect, so we understand the current draft and read preliminary implementation. We can see that some code style are inappropriate, or even cause confusion. Therefore, this PR try to correct it.

Why are the changes needed?

Fix code style on scala server side.

Does this PR introduce any user-facing change?

'No'.
Spark Connect just started.

How was this patch tested?

N/A

@amaliujia
Copy link
Contributor

amaliujia commented Sep 30, 2022

It's ok to fix code style on, for example, scala server side.

I want to hold a bit on the proto side. The proto itself will evolve quickly and itself is not at a stable position now. Whatever we change now could be no longer useful soon. Plus every time we update the proto we need to update the generate file on the python side. We are planing to change the python side with some sort of automatic way for python proto generation.

Before that is achieved, the idea could be that we limit changes on the proto to only core API coverage to unblock other pieces (DataFrame API support in clients etc.)

@beliefer
Copy link
Contributor Author

beliefer commented Oct 8, 2022

Before that is achieved, the idea could be that we limit changes on the proto to only core API coverage to unblock other pieces (DataFrame API support in clients etc.)

Thank you. I will revert the changes on the proto.

@beliefer beliefer force-pushed the SPARK-40448_followup branch from f176cdb to 59b22cc Compare October 8, 2022 01:44
@beliefer beliefer changed the title [SPARK-40448][CONNECT][FOLLOWUP] Use more suitable message name. [SPARK-40448][CONNECT][FOLLOWUP] Use more suitable variable name and fix code style. Oct 8, 2022
Copy link
Contributor

@amaliujia amaliujia left a comment

Choose a reason for hiding this comment

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

LGTM!

@beliefer
Copy link
Contributor Author

beliefer commented Oct 8, 2022

ping @grundprinzip

@beliefer
Copy link
Contributor Author

beliefer commented Oct 9, 2022

cc @dongjoon-hyun @HyukjinKwon

@HyukjinKwon
Copy link
Member

Merged to master.

@beliefer
Copy link
Contributor Author

@amaliujia @grundprinzip @HyukjinKwon Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants