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

[SPARK-47921][CONNECT] Fix ExecuteJobTag creation in ExecuteHolder #46140

Closed

Conversation

allisonwang-db
Copy link
Contributor

@allisonwang-db allisonwang-db commented Apr 19, 2024

What changes were proposed in this pull request?

This PR fixes a bug in the ExecuteJobTag creation in ExecuteHolder. The sessionId and userId are reversed.

object ExecuteJobTag {
private val prefix = "SparkConnect_OperationTag"
def apply(sessionId: String, userId: String, operationId: String): String = {

Why are the changes needed?

To fix a bug

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing test

Was this patch authored or co-authored using generative AI tooling?

No

@allisonwang-db
Copy link
Contributor Author

cc @jasonli-db @HyukjinKwon

@ueshin
Copy link
Member

ueshin commented Apr 23, 2024

We may want to rather fix the arguments of apply function to be consistent with the usage of them and with the other similar functions:

object ExecuteJobTag {
private val prefix = "SparkConnect_OperationTag"
def apply(sessionId: String, userId: String, operationId: String): String = {
s"${prefix}_" +
s"User_${userId}_" +
s"Session_${sessionId}_" +
s"Operation_${operationId}"
}

object ExecuteSessionTag {
private val prefix = "SparkConnect_SessionTag"
def apply(userId: String, sessionId: String, tag: String): String = {
ProtoUtils.throwIfInvalidTag(tag)
s"${prefix}_" +
s"User_${userId}_" +
s"Session_${sessionId}_" +
s"Tag_${tag}"
}

@ueshin
Copy link
Member

ueshin commented Apr 23, 2024

Also cc @gengliangwang @juliuszsompolski

@@ -37,7 +37,7 @@ class SparkConnectServerListenerSuite

private var kvstore: ElementTrackingStore = _

private val jobTag = ExecuteJobTag("sessionId", "userId", "operationId")
private val jobTag = ExecuteJobTag("userId", "sessionId", "operationId")
Copy link
Member

Choose a reason for hiding this comment

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

From the code changes, there does seem to be a bug. All the callers are using the correct order. This PR seems just to unify the usages for such functions.

Copy link
Member

Choose a reason for hiding this comment

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

The original fix is dd6636b.
I'm fine with it too, but I asked #46140 (comment) because I worried the inconsistency with the similar API.

Copy link
Contributor

@juliuszsompolski juliuszsompolski left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for spotting this.

@ueshin
Copy link
Member

ueshin commented Apr 25, 2024

Thanks! merging to master/3.5.

@ueshin ueshin closed this in 5a1559a Apr 25, 2024
ueshin pushed a commit that referenced this pull request Apr 25, 2024
### What changes were proposed in this pull request?

This PR fixes a bug in the ExecuteJobTag creation in ExecuteHolder. The sessionId and userId are reversed.

https://github.com/apache/spark/blob/8aa8ad6be7b3eeceafa2ad1e9211fb8133bb675c/connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteHolder.scala#L296-L299

### Why are the changes needed?

To fix a bug

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

Existing test

### Was this patch authored or co-authored using generative AI tooling?

No

Closes #46140 from allisonwang-db/spark-47921-execute-job-tag.

Authored-by: allisonwang-db <allison.wang@databricks.com>
Signed-off-by: Takuya UESHIN <ueshin@databricks.com>
(cherry picked from commit 5a1559a)
Signed-off-by: Takuya UESHIN <ueshin@databricks.com>
JacobZheng0927 pushed a commit to JacobZheng0927/spark that referenced this pull request May 11, 2024
### What changes were proposed in this pull request?

This PR fixes a bug in the ExecuteJobTag creation in ExecuteHolder. The sessionId and userId are reversed.

https://github.com/apache/spark/blob/8aa8ad6be7b3eeceafa2ad1e9211fb8133bb675c/connector/connect/server/src/main/scala/org/apache/spark/sql/connect/service/ExecuteHolder.scala#L296-L299

### Why are the changes needed?

To fix a bug

### Does this PR introduce _any_ user-facing change?

No

### How was this patch tested?

Existing test

### Was this patch authored or co-authored using generative AI tooling?

No

Closes apache#46140 from allisonwang-db/spark-47921-execute-job-tag.

Authored-by: allisonwang-db <allison.wang@databricks.com>
Signed-off-by: Takuya UESHIN <ueshin@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants