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

Improve array_concat signature for null and empty array #8594

Closed
wants to merge 3 commits into from

Conversation

jayzhan211
Copy link
Contributor

@jayzhan211 jayzhan211 commented Dec 20, 2023

Which issue does this PR close?

Closes #.
Part of #7142

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) labels Dec 20, 2023
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
@@ -122,6 +122,9 @@ pub enum TypeSignature {
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
ArrayAndElement,
/// Specialized Signature for ArrayConcat
/// Accept arbitrary arguments but they SHOULD be List/LargeList or Null, and the list dimension MAY NOT be the same.
ArrayConcat,
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any usecase for this type of signature other than ArrayConcat? If there is only a single function that would likely have this signature, the code probably doesn't belong in the TypeSignature enum

I don't fully understand

and the list dimension MAY NOT be the same.

For example, one of the tests is

select array_concat([1, null], [null]);

Doesn't that have two arguments of the same dimension?

Copy link
Contributor Author

@jayzhan211 jayzhan211 Jan 7, 2024

Choose a reason for hiding this comment

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

In this case, where should be handled arrayConcat? coerce argument for fun?. I think there is no other array function to share the same coercion with arrayConcat

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ist dimension MAY NOT be the same.
I mean MAY or MAY NOT.
array concat is able to concat different dimension array in DF.

select array_concat([1, null], [null]); is the normal cases, dimensions are both 1.
select array_concat([1, null], null); is also valid, it acts like array_append, where dimensions are 1 and 0.

Copy link
Member

@Weijun-H Weijun-H Jan 20, 2024

Choose a reason for hiding this comment

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

When I worked on #8902, I thought this pr would benefit functions like array_union, and array_intersect. Their arguments are array and array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Weijun-H But array_concat accepts list dimension with 1 and 3 (any n and m). union, intersect should be n and n-1

Copy link

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale PR has not had any activity for some time label Apr 14, 2024
@jayzhan211 jayzhan211 closed this Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logical-expr Logical plan and expressions physical-expr Physical Expressions sqllogictest SQL Logic Tests (.slt) Stale PR has not had any activity for some time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants