Skip to content

[SPARK-53354][CONNECT] Simplify LiteralValueProtoConverter.toCatalystStruct #52098

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

Closed
wants to merge 1 commit into from

Conversation

heyihong
Copy link
Contributor

@heyihong heyihong commented Aug 22, 2025

What changes were proposed in this pull request?

This PR simplifies the LiteralValueProtoConverter.toCatalystStruct method by refactoring the struct conversion logic to be more straightforward and maintainable. The main changes include:

  1. Simplified return type: Changed toCatalystStruct to return only the converted struct value instead of a tuple (Any, proto.DataType.Struct)
  2. Extracted struct type resolution: Created a new getProtoStructType method to handle struct type resolution separately
  3. Simplified internal conversion: Introduced toCatalystStructInternal method that takes the struct type as a parameter
  4. Removed complex type inference logic: Eliminated the LiteralValueWithDataType case class and simplified the getConverter method by removing the inferDataType parameter
  5. Enhanced recursive type inference: Improved the getInferredDataType method to support recursive type inference for struct fields

Why are the changes needed?

The original method is a bit overly complex with multiple code paths and conditional logic that made it difficult to understand and maintain. This refactoring improves code readability while preserving the same functionality.

Does this PR introduce any user-facing change?

No. This is a pure refactoring that maintains the same external behavior and API.

How was this patch tested?

build/sbt "connect/testOnly *LiteralExpressionProtoConverterSuite"

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

Generated-by: Cursor 1.4.5

@heyihong
Copy link
Contributor Author

@zhengruifeng

@heyihong heyihong changed the title [SPARK-53354] Simplify LiteralValueProtoConverter.toCatalystStruct [SPARK-53354][CONNECT] Simplify LiteralValueProtoConverter.toCatalystStruct Aug 22, 2025
@heyihong heyihong force-pushed the SPARK-53354 branch 2 times, most recently from 5425a8d to eba8c85 Compare August 22, 2025 19:49
@zhengruifeng
Copy link
Contributor

thanks, merged to master

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.

2 participants