Skip to content

Conversation

@Weijun-H
Copy link
Member

@Weijun-H Weijun-H commented Jan 3, 2024

Which issue does this PR close?

Parts #8185

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels Jan 3, 2024
@Weijun-H Weijun-H mentioned this pull request Jan 3, 2024
19 tasks
.collect::<Result<UInt64Array>>()?;

let result = match &args[0].data_type() {
DataType::List(_) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Although this implementation is small, I think having a closure for generic array is still a good practice.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it would be better to avoid the duplication if possible

@Weijun-H Weijun-H requested review from alamb and jayzhan211 January 4, 2024 03:00
Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

LGTM

let list_array = as_large_list_array(&args[0])?;
generic_list_cardinality::<i64>(list_array)
}
_ => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_ => {
other => {
exec_err!(
"cardinality does not support type '{:?}'",
other
)
}

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good to me -- thanks @Weijun-H

I think @comphead had a suggestion for improvement, but we could also do it as a follow on PR as well

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

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

lgtm thanks @Weijun-H

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

Labels

physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants