Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Jan 8, 2024

What changes were proposed in this pull request?

The pr aims to support s.c.immutable.ArraySeq as customCollectionCls in ArrowDeserializers.

Why are the changes needed?

Because s.c.immutable.ArraySeq is a commonly used type in Scala 2.13, we should support it.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Update existed UT (SQLImplicitsTestSuite).

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

No.


val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
immutable.ArraySeq.unsafeWrapArray[Int](a.appended(5).appended(6).toArray))
immutable.ArraySeq.unsafeWrapArray[Int]((a :+ 5 :+ 6).toArray))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only fix the comment related issues of another PR: #44591

Copy link
Contributor

@LuciferYang LuciferYang Jan 8, 2024

Choose a reason for hiding this comment

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

I think this should be a follow up of #44591

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we move this change to a separate pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

testImplicit(booleans)
testImplicit(booleans.toSeq)
testImplicit(booleans.toSeq)(newBooleanSeqEncoder)
testImplicit(immutable.ArraySeq.unsafeWrapArray(booleans))
Copy link
Contributor

Choose a reason for hiding this comment

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

we can just

import org.apache.spark.util.ArrayImplicits._

at the file header, and then

testImplicit(booleans.toImmutableArraySeq)
testImplicit(shorts.toImmutableArraySeq)
...

mutable.ArraySeq.make(array.asInstanceOf[Array[T]])
}

def toImmutableArraySeq[T](array: AnyRef): immutable.ArraySeq[T] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Perhaps we can directly use the implicit conversion defined in ArrayImplicits instead of add this new function, which is now a more generic conversion way in Spark code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


val myUdf2 = udf((a: immutable.ArraySeq[Int]) =>
immutable.ArraySeq.unsafeWrapArray[Int](a.appended(5).appended(6).toArray))
immutable.ArraySeq.unsafeWrapArray[Int]((a :+ 5 :+ 6).toArray))
Copy link
Contributor

@LuciferYang LuciferYang Jan 8, 2024

Choose a reason for hiding this comment

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

I think this should be a follow up of #44591

@panbingkun panbingkun marked this pull request as ready for review January 8, 2024 11:33
@LuciferYang
Copy link
Contributor

Merged into master for Spark 4.0. Thanks @panbingkun @srowen

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