Skip to content

Conversation

@jayzhan211
Copy link
Contributor

Which issue does this PR close?

Part of #7988
Closes #8145

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 Nov 14, 2023
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
Signed-off-by: jayzhan211 <[email protected]>
@jayzhan211 jayzhan211 force-pushed the array-position-revisit branch from 385be3c to e46a627 Compare November 14, 2023 13:51
Signed-off-by: jayzhan211 <[email protected]>
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.

This certainly seems like an improvement to me. I think we can probably make it even more efficient but doing that as a follow on PR would make a lot of sense too. Thank you @jayzhan211

cc @Veeupup in case you have some other thoughts

}};
}

/// Computes a BooleanArray indicating equality or inequality between elements in a list array and a specified element array.
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

eq: bool,
) -> Result<BooleanArray> {
let indices = UInt32Array::from(vec![row_index as u32]);
let element_array_row = arrow::compute::take(element_array, &indices, None)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will always be a single row Array as indices have a single value 🤔 I wonder if you could call

I think you could instead call list_array_row.values().slice() and find the relevant row to compare against those values 🤔

.filter(|(_, x)| *x == el)
.flat_map(|(i, _)| Some((i + 1) as u64))
.collect::<UInt64Array>();
fn general_position<OffsetSize: OffsetSizeTrait>(
Copy link
Contributor

@Veeupup Veeupup Nov 15, 2023

Choose a reason for hiding this comment

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

nit: if we only use i32 as OffsetSize, do we really need the Generics?

Copy link
Contributor Author

@jayzhan211 jayzhan211 Nov 15, 2023

Choose a reason for hiding this comment

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

no, but we will need it if we want to extend it for large list, I'm just lazy to remove it for now :)

Copy link
Member

Choose a reason for hiding this comment

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

Nice extension 👍

Copy link
Contributor

@Veeupup Veeupup left a comment

Choose a reason for hiding this comment

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

LGTM : )

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.

Generalized array_position to get at most n element and start from certain position.

4 participants