Skip to content

support LargeList for arrow_cast, support ScalarValue::LargeList - #8290

Merged
alamb merged 8 commits into
apache:mainfrom
Weijun-H:scalar-largelist
Nov 22, 2023
Merged

support LargeList for arrow_cast, support ScalarValue::LargeList#8290
alamb merged 8 commits into
apache:mainfrom
Weijun-H:scalar-largelist

Conversation

@Weijun-H

@Weijun-HWeijun-H commented Nov 21, 2023

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #.

Rationale for this change

It is hard to test LargeList in sqllogictest #8121, the convenient idea is to use arrow_cast() instead of make_array

What changes are included in this PR?

  • support LargeList for arrow_cast
  • support ScalarValue::LargeList
  • support ScalarValue::new_large_list
  • support LargeList for protobuf

Are these changes tested?

Are there any user-facing changes?

@alambalamb left a comment

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.

Thank you @Weijun-H -- this looks very nice 👍

@alambalamb changed the title support LargeList for arrow_castsupport LargeList for arrow_cast, support ScalarValue::LargeListNov 21, 2023
}
}
(LargeList(arr1), LargeList(arr2)) => {
if arr1.data_type() == arr2.data_type() {

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.

@Weijun-H can you try (in a follow on PR) to use the arrow lt kernels diectly on the list (rather than calling it on each element)? I can't remember if the kernel knows how to handle things

if list_arr1.len() != list_arr2.len() {
return None;
}
for i in 0..list_arr1.len() {

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.

I think the list lengths should always be 1, so I don't really understand why this is iterating (though I see it is just following the pattern of ListArray)

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.

Maybe you can clean it up in a follow on PR (by asserting the list is length 1)

@Weijun-HWeijun-HNov 22, 2023

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I will work on this on the next PR

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I think @jayzhan211 is working on this #8253

@alamb
alamb merged commit f2b0344 into apache:mainNov 22, 2023
@alamb

Copy link
Copy Markdown
Contributor

Thanks again @Weijun-H

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

Labels

sqlSQL PlannersqllogictestSQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@Weijun-H@alamb