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

Use new DecimalArray creation API in arrow crate #1249

Merged
merged 9 commits into from
Feb 15, 2022

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Jan 30, 2022

Builds on #1223 so draft until that is done

Rationale:

#1223 introduces a more performant and idiomatic API for creating DecimalArrays than DecimalBuilder so update the code to use that.

t
})
.collect::<Result<DecimalArray>>()?
// PERF: we could avoid re-validating that the data in
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this re-validation occurs in DecimalBuilder as well, so this change isn't worse. I just noticed it while working on the code and figured I would leave a hint for future readers

@codecov-commenter
Copy link

codecov-commenter commented Jan 30, 2022

Codecov Report

Merging #1249 (11f1832) into master (35e16be) will decrease coverage by 0.01%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1249      +/-   ##
==========================================
- Coverage   83.04%   83.03%   -0.02%     
==========================================
  Files         180      180              
  Lines       52424    52365      -59     
==========================================
- Hits        43537    43479      -58     
+ Misses       8887     8886       -1     
Impacted Files Coverage Δ
arrow/src/compute/kernels/cast.rs 95.30% <86.66%> (+0.09%) ⬆️
arrow/src/compute/kernels/take.rs 95.27% <93.75%> (-0.08%) ⬇️
arrow/src/array/array_binary.rs 93.47% <100.00%> (-0.06%) ⬇️
arrow/src/array/equal/mod.rs 93.22% <100.00%> (-0.06%) ⬇️
arrow/src/array/equal_json.rs 89.70% <100.00%> (-0.21%) ⬇️
arrow/src/array/transform/mod.rs 84.52% <100.00%> (+<0.01%) ⬆️
arrow/src/compute/kernels/sort.rs 95.11% <100.00%> (-0.02%) ⬇️
arrow/src/ffi.rs 84.53% <100.00%> (-0.17%) ⬇️
arrow/src/util/pretty.rs 96.53% <100.00%> (-0.13%) ⬇️
arrow/src/datatypes/field.rs 53.79% <0.00%> (-0.31%) ⬇️
... and 3 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 35e16be...11f1832. Read the comment docs.

@alamb alamb force-pushed the alamb/clean_up_decimal_creation2 branch from f64a3ea to 11f1832 Compare February 8, 2022 20:14
@alamb alamb marked this pull request as ready for review February 8, 2022 20:14
@alamb
Copy link
Contributor Author

alamb commented Feb 8, 2022

FYI @liukun4515

@liukun4515
Copy link
Contributor

FYI @liukun4515

I will review this later in this weekend.

Copy link
Contributor

@liukun4515 liukun4515 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 for your refactor. @alamb

@alamb
Copy link
Contributor Author

alamb commented Feb 15, 2022

Thanks @liukun4515

@alamb alamb merged commit 747e72a into apache:master Feb 15, 2022
@alamb alamb deleted the alamb/clean_up_decimal_creation2 branch February 15, 2022 20:24
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.

None yet

3 participants