Skip to content

[SPARK-43321][Connect][Followup] Better names for APIs used in Scala Client joinWith#41971

Closed
zhenlineo wants to merge 2 commits intoapache:masterfrom
zhenlineo:followup-joinwith
Closed

[SPARK-43321][Connect][Followup] Better names for APIs used in Scala Client joinWith#41971
zhenlineo wants to merge 2 commits intoapache:masterfrom
zhenlineo:followup-joinwith

Conversation

@zhenlineo
Copy link
Contributor

What changes were proposed in this pull request?

Directly use struct in the names to directly call out the underlying encoder is composed using a struct encoder.

Why are the changes needed?

Addressing PR review feedbacks of #40997

Does this PR introduce any user-facing change?

No

How was this patch tested?

Existing tests

@zhenlineo
Copy link
Contributor Author

cc @hvanhovell

Copy link
Contributor

@vicennial vicennial left a comment

Choose a reason for hiding this comment

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

LGTM

override def schema: StructType = StructType(StructField("value", dataType, nullable) :: Nil)
def lenientSerialization: Boolean = false
def isFlattenable: Boolean = false
def isStruct: Boolean = false // set to true if it is a struct encoder
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: comment seems a bit redundant

@@ -103,18 +102,18 @@ object AgnosticEncoders {
}

// Contains a sequence of fields. The fields can be flattened to columns in a row.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can also remove the word "flattened" in this comment here I think

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

LGTM

@zhenlineo
Copy link
Contributor Author

cc @HyukjinKwon Can we merge this? Thanks.

@yaooqinn yaooqinn closed this in fbb85ac Jul 13, 2023
@yaooqinn
Copy link
Member

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.

4 participants