Skip to content

[SPARK-47736][SQL] Add support for AbstractArrayType#45891

Closed
mihailomilosevic2001 wants to merge 15 commits into
apache:masterfrom
mihailomilosevic2001:SPARK-47736
Closed

[SPARK-47736][SQL] Add support for AbstractArrayType#45891
mihailomilosevic2001 wants to merge 15 commits into
apache:masterfrom
mihailomilosevic2001:SPARK-47736

Conversation

@mihailomilosevic2001
Copy link
Copy Markdown
Contributor

@mihailomilosevic2001 mihailomilosevic2001 commented Apr 5, 2024

What changes were proposed in this pull request?

Addition of abstract arraytype which accepts StringTypeCollated as elementType. Changes in this PR #45693 work for ArrayJoin, but will not work in general for other functions. This PR introduces a change to give an interface for all functions. Merge only after #45693.

Why are the changes needed?

This is needed in order to enable functions to use collated arrays.

Does this PR introduce any user-facing change?

Yes, collation functions will work.

How was this patch tested?

Test for array_join added to CollationSuite

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

No.

@github-actions github-actions Bot added the SQL label Apr 5, 2024
Copy link
Copy Markdown
Contributor

@uros-db uros-db left a comment

Choose a reason for hiding this comment

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

lgtm

@mihailomilosevic2001
Copy link
Copy Markdown
Contributor Author

@cloud-fan Do you think adding AbstractArrayType could be a good idea as a general and not only for StringTypeCollated? Otherwise this PR should be ready for review.

* Use ArrayType(StringTypeCollated) for expressions supporting different type of
* collations in ArrayTypes
*/
case class AbstractArrayType(elementType: StringTypeCollated) extends AbstractDataType {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we make it general to take AbstractDataType? I don't think we should limit it to string type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, I also realised, should I move the StringTypeCollated to the right package? It is now in catalyst.expressions instead of types as all other types.

@mihailomilosevic2001 mihailomilosevic2001 changed the title Add support for AbstractArrayType(StringTypeCollated) [SPARK-47736][SQL] Add support for AbstractArrayType Apr 9, 2024
# Conflicts:
#	sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/collectionOperations.scala
Comment thread sql/api/src/main/scala/org/apache/spark/sql/types/ArrayType.scala Outdated
@mihailomilosevic2001
Copy link
Copy Markdown
Contributor Author

@cloud-fan Did some refactoring with some renaming, so I re-requested approve

Comment thread sql/api/src/main/scala/org/apache/spark/sql/types/ArrayType.scala Outdated
@cloud-fan
Copy link
Copy Markdown
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in a8b919f Apr 11, 2024
import org.apache.spark.sql.types.{AbstractDataType, DataType, StringType}

/**
* StringTypeCollated is an abstract class for StringType with collation support.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

missed a docstring update here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Noted, will include the change with some of the next PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants