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

Open
wants to merge 1 commit into
base: master
Choose a base branch
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)

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.

3 participants