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

Allow casting Timestamp arrays into String #664

Merged
merged 1 commit into from
Aug 16, 2021

Conversation

sum12
Copy link
Contributor

@sum12 sum12 commented Aug 5, 2021

Which issue does this PR close?

Closes #587

Rationale for this change

What changes are included in this PR?

there are two commits included in the PR
the first commit shows how it should be possible to implement. But the commit fails to compile with

error[E0599]: the method `value_as_datetime` exists for reference `&array_primitive::PrimitiveArray<T>`, but its trait bounds were not satisfied
    --> arrow/src/compute/kernels/cast.rs:1044:48
     |
1044 |     (0..array.len()).for_each(|ix| match array.value_as_datetime(ix) {
     |                                                ^^^^^^^^^^^^^^^^^ method cannot be called on `&array_primitive::PrimitiveArray<T>` due to unsatisfied trait bounds
     |
     = note: the following trait bounds were not satisfied:
             `T: numeric::ArrowNumericType`

error[E0599]: the method `value_as_datetime` exists for reference `&array_primitive::PrimitiveArray<T>`, but its trait bounds were not satisfied
    --> arrow/src/compute/kernels/cast.rs:1044:48
     |
1044 |     (0..array.len()).for_each(|ix| match array.value_as_datetime(ix) {
     |                                                ^^^^^^^^^^^^^^^^^ method cannot be called on `&array_primitive::PrimitiveArray<T>` due to unsatisfied trait bounds
     |
     = note: the following trait bounds were not satisfied:
             `T: numeric::ArrowNumericType`

error: aborting due to previous error

and compiles fine with second commit. But it feels not correct to use the second commit as the ArrowNumericType and Timestamp* types are not really releated. (Or I do not understand them correctly)

Are there any user-facing changes?

yes (additional ways to cast array) (where do I document this ?)
no breaking changes

@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 5, 2021
@alamb
Copy link
Contributor

alamb commented Aug 6, 2021

Thanks @sum12 -- I plan to check this out tomorrow

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 is looking good @sum12 . I left some style comments -- but the only thing this PR really needs to be good to go is some basic tests. Perhaps you can follow the style of the existing tests?

arrow/src/compute/kernels/cast.rs Outdated Show resolved Hide resolved
@sum12 sum12 force-pushed the value_as_datetime branch 2 times, most recently from 1675482 to 1f996d8 Compare August 13, 2021 07:14
#[test]
fn test_cast_timestamp_to_string() {
let a = TimestampMillisecondArray::from_opt_vec(
vec![Some(864000000005), Some(1545696000001), None],
Copy link
Contributor

Choose a reason for hiding this comment

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

happy that we keep nulls now

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 @alamb I'll leave it to you to have a final look + merge

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2021

Codecov Report

Merging #664 (d9de2de) into master (b38a4b6) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #664      +/-   ##
==========================================
- Coverage   82.50%   82.45%   -0.05%     
==========================================
  Files         168      168              
  Lines       47237    47379     +142     
==========================================
+ Hits        38971    39066      +95     
- Misses       8266     8313      +47     
Impacted Files Coverage Δ
arrow/src/compute/kernels/cast.rs 94.63% <100.00%> (+0.09%) ⬆️
arrow/src/array/transform/utils.rs 95.00% <0.00%> (-5.00%) ⬇️
arrow/src/array/equal_json.rs 85.21% <0.00%> (-3.48%) ⬇️
arrow/src/tensor.rs 85.00% <0.00%> (-2.50%) ⬇️
parquet/src/column/page.rs 97.36% <0.00%> (-1.32%) ⬇️
arrow/src/array/equal/utils.rs 74.00% <0.00%> (-1.00%) ⬇️
parquet/src/record/api.rs 91.60% <0.00%> (-0.88%) ⬇️
parquet/src/file/statistics.rs 93.80% <0.00%> (-0.83%) ⬇️
arrow/src/csv/writer.rs 82.41% <0.00%> (-0.74%) ⬇️
arrow/src/array/array_map.rs 81.50% <0.00%> (-0.69%) ⬇️
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b38a4b6...d9de2de. Read the comment docs.

the change adds uses the existing `PrimitiveArray::value_as_datetime` to
support casting from `Timestamp(_,_)` to ``[Large]Utf8`.
@sum12 sum12 force-pushed the value_as_datetime branch from 1f996d8 to d9de2de Compare August 13, 2021 10:04
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 looks great ! Thank you again for your contribution @sum12 -- I am sorry for the delay in reviewing.

@alamb alamb merged commit f569e8f into apache:master Aug 16, 2021
alamb pushed a commit that referenced this pull request Aug 19, 2021
the change adds uses the existing `PrimitiveArray::value_as_datetime` to
support casting from `Timestamp(_,_)` to ``[Large]Utf8`.
alamb added a commit that referenced this pull request Aug 20, 2021
the change adds uses the existing `PrimitiveArray::value_as_datetime` to
support casting from `Timestamp(_,_)` to ``[Large]Utf8`.

Co-authored-by: Sumit <[email protected]>
ovr pushed a commit to cube-js/arrow-rs that referenced this pull request Aug 25, 2022
…e#698)

the change adds uses the existing `PrimitiveArray::value_as_datetime` to
support casting from `Timestamp(_,_)` to ``[Large]Utf8`.

Co-authored-by: Sumit <[email protected]>
ovr pushed a commit to cube-js/arrow-rs that referenced this pull request Aug 25, 2022
…e#698)

the change adds uses the existing `PrimitiveArray::value_as_datetime` to
support casting from `Timestamp(_,_)` to ``[Large]Utf8`.

Co-authored-by: Sumit <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't cast from timestamp array to string array
4 participants