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-50979][CONNECT] Remove .expr/.typedExpr implicits #49657

Closed
wants to merge 5 commits into from

Conversation

hvanhovell
Copy link
Contributor

What changes were proposed in this pull request?

This PR removed the .expr/.typedExpr Column conversion implicits from the Connect client.

Why are the changes needed?

Code clean-up.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

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

No.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, this looks like a safe clean-up. BTW, are these all of them, @hvanhovell ?

@vrozov
Copy link
Member

vrozov commented Jan 24, 2025

Based on failed test, there may be user facing change as plan changes.

@dongjoon-hyun
Copy link
Member

Oh, ya. The plan becomes different.

[info] - hint *** FAILED *** (11 milliseconds)
[info]   Expected and actual plans do not match:
[info]   
[info]   === Expected Plan (with excess fields trimmed) ===
[info]   common {
[info]     plan_id: 1
[info]   }
[info]   hint {
[info]     input {
[info]       common {
[info]         plan_id: 0
[info]       }
[info]       local_relation {
[info]         schema: "struct<id:bigint,a:int,b:double>"
[info]       }
[info]     }
[info]     name: "coalesce"
[info]     parameters {
[info]       literal {
[info]         integer: 100
[info]       }
[info]       common {
[info]         origin {
[info]           jvm_origin {
[info]             stack_trace {
[info]               class_loader_name: "app"
[info]               declaring_class: "org.apache.spark.sql.connect.Dataset"
[info]               method_name: "~~trimmed~anonfun~~"
[info]               file_name: "Dataset.scala"
[info]             }
[info]             stack_trace {
[info]               class_loader_name: "app"
[info]               declaring_class: "org.apache.spark.sql.connect.SparkSession"
[info]               method_name: "newDataset"
[info]               file_name: "SparkSession.scala"
[info]             }
[info]           }
[info]         }
[info]       }
[info]     }
[info]   }
[info]   
[info]   
[info]   === Actual Plan (with excess fields trimmed) ===
[info]   common {
[info]     plan_id: 1
[info]   }
[info]   hint {
[info]     input {
[info]       common {
[info]         plan_id: 0
[info]       }
[info]       local_relation {
[info]         schema: "struct<id:bigint,a:int,b:double>"
[info]       }
[info]     }
[info]     name: "coalesce"
[info]     parameters {
[info]       literal {
[info]         integer: 100
[info]       }
[info]       common {
[info]         origin {
[info]           jvm_origin {
[info]             stack_trace {
[info]               class_loader_name: "app"
[info]               declaring_class: "org.apache.spark.sql.functions$"
[info]               method_name: "lit"
[info]               file_name: "functions.scala"
[info]             }
[info]             stack_trace {
[info]               class_loader_name: "app"
[info]               declaring_class: "org.apache.spark.sql.connect.ColumnNodeToProtoConverter$"
[info]               method_name: "toLiteral"
[info]               file_name: "columnNodeSupport.scala"
[info]             }
[info]           }
[info]         }
[info]       }
[info]     }
[info]   } (PlanGenerationTestSuite.scala:160)

@vrozov
Copy link
Member

vrozov commented Jan 29, 2025

Please update "Does this PR introduce any user-facing change?"

@hvanhovell
Copy link
Contributor Author

@vrozov I does not really introduce a user facing change. I actually improved the origin for these cases.

@hvanhovell
Copy link
Contributor Author

Merging.

@asfgit asfgit closed this in 6bbfa2d Jan 29, 2025
asfgit pushed a commit that referenced this pull request Jan 29, 2025
### What changes were proposed in this pull request?
This PR removed the .expr/.typedExpr Column conversion implicits from the Connect client.

### Why are the changes needed?
Code clean-up.

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

### How was this patch tested?
Existing tests.

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

Closes #49657 from hvanhovell/SPARK-50979.

Authored-by: Herman van Hovell <[email protected]>
Signed-off-by: Herman van Hovell <[email protected]>
(cherry picked from commit 6bbfa2d)
Signed-off-by: Herman van Hovell <[email protected]>
@dongjoon-hyun
Copy link
Member

Thank you, @hvanhovell .

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.

5 participants