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.

Sign up for GitHub

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

Jump to bottom

support cast decimal to decimal #1084

Merged
merged 4 commits into from
Dec 23, 2021
Merged

support cast decimal to decimal #1084

merged 4 commits into from
Dec 23, 2021

Conversation

liukun4515
Copy link
Contributor

Which issue does this PR close?

part of #1043

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Dec 22, 2021
@liukun4515 liukun4515 mentioned this pull request Dec 22, 2021
Closed
8 tasks
@codecov-commenter
Copy link

codecov-commenter commented Dec 22, 2021 •
edited
Loading

Codecov Report

Merging #1084 (d15880f) into master (e8cc39e) will increase coverage by 0.00%.
The diff coverage is 86.48%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1084   +/-   ##
=======================================
  Coverage   82.31%   82.31%           
=======================================
  Files         168      168           
  Lines       49383    49420   +37     
=======================================
+ Hits        40649    40681   +32     
- Misses       8734     8739    +5
Impacted Files Coverage Δ
arrow/src/compute/kernels/cast.rs 95.06% <86.48%> (-0.17%) ⬇️
arrow/src/datatypes/datatype.rs 66.38% <0.00%> (-0.43%) ⬇️
arrow/src/datatypes/field.rs 53.37% <0.00%> (-0.31%) ⬇️
arrow/src/array/transform/mod.rs 84.86% <0.00%> (+0.13%) ⬆️
parquet/src/encodings/encoding.rs 93.71% <0.00%> (+0.19%) ⬆️

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 e8cc39e...d15880f. Read the comment docs.

alamb
alamb reviewed Dec 22, 2021
View reviewed changes
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.

@liukun4515 I looked at this PR and the code looks good. I think all it needs now is some tests and a rebase and it would be good to go.

I plan to create an arrow 6.5.0 release candidate tomorrow (Thursday) -- let me know if you think we should aim to get this PR merged before doing so.

arrow/src/compute/kernels/cast.rs Outdated
let input_type = DataType::Decimal(20, 3);
let output_type = DataType::Decimal(20, 4);
assert!(can_cast_types(&input_type, &output_type));
// let array = vec![Some(1123456), Some(2123456), Some(3123456), None];
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we just need some tests here, and we'll be all set 👍

@liukun4515 liukun4515 marked this pull request as ready for review December 23, 2021 02:17
@liukun4515
Copy link
Contributor Author

@liukun4515 I looked at this PR and the code looks good. I think all it needs now is some tests and a rebase and it would be good to go.

I plan to create an arrow 6.5.0 release candidate tomorrow (Thursday) -- let me know if you think we should aim to get this PR merged before doing so.

I rebased this branch with master and added some test cases.
If it looks good for you, you can merge it.
Thank.

@liukun4515 liukun4515 requested a review from alamb December 23, 2021 02:34
@alamb alamb merged commit 8b85aa0 into apache:master Dec 23, 2021
@alamb
Copy link
Contributor

alamb commented Dec 23, 2021

Thanks @liukun4515 ❤️

alamb pushed a commit that referenced this pull request Dec 23, 2021
@liukun4515 @alamb
support cast decimal to decimal (#1084)
a0560ec 
* support cast decimal to decimal

* add test case

* remove meaningless code
@alamb alamb mentioned this pull request Dec 23, 2021
Merged
alamb added a commit that referenced this pull request Dec 23, 2021
@alamb @liukun4515
support cast decimal to decimal (#1084) (#1093)
092fc64 
* support cast decimal to decimal

* add test case

* remove meaningless code

Co-authored-by: Kun Liu <liukun@apache.org>
@liukun4515 liukun4515 deleted the cast_decimal_to_decimal branch December 24, 2021 01:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houqp houqp houqp approved these changes

@alamb alamb Awaiting requested review from alamb

Assignees
No one assigned
Labels
arrow Changes to the arrow crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@liukun4515 @codecov-commenter @alamb @houqp