Skip to content

Conversation

@ritchie46
Copy link
Contributor

Sorry that the PR's are not more clustered, but they occur to me in the wild.

This PR allows casting from LargeUtf8 to numerical and temporal types. It also modifies the already existing string to temporal casts such that it uses the new faster from_trusted_length iterator API.

@ritchie46 ritchie46 force-pushed the large_utf8_to_numerical branch from ad24ce3 to cda7de3 Compare February 25, 2021 08:34
Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link

@nevi-me
Copy link
Contributor

nevi-me commented Feb 25, 2021

@ritchie46 you have unused imports somewhere

@nevi-me
Copy link
Contributor

nevi-me commented Feb 26, 2021

I'll fix the clippy lints, and merge this after #9425

@ritchie46
Copy link
Contributor Author

@nevi-me thanks. Sorry about the lints, I totally forgot them today.

@nevi-me nevi-me force-pushed the large_utf8_to_numerical branch from 3ca7ead to 312c082 Compare February 26, 2021 20:04
@alamb
Copy link
Contributor

alamb commented Feb 26, 2021

The integration test failure seems like it is unrelated to the changes in this PR: https://issues.apache.org/jira/browse/ARROW-11717


#[test]
fn test_cast_string_to_timestamp() {
let a = StringArray::from(vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend adding coverage here to convert from LargeUtf8 string arrays as well - I don't think they are covered by these tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nevi-me I just fixed this test. I already modified the test with that intention but I apparently tested stringarray twice instead of stringarray and largestringarray

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @ritchie46 !

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants