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

Feat: arrow csv decimal256 #3711

Merged
merged 9 commits into from Feb 14, 2023

Conversation

suxiaogang223
Copy link
Contributor

Which issue does this PR close?

Closes #3474

Rationale for this change

arrow-csv only seems to support decimal128

What changes are included in this PR?

now the arrow-csv can support decimal256

Are there any user-facing changes?

nothing

There is still room for improvement in the code
@github-actions github-actions bot added the arrow Changes to the arrow crate label Feb 13, 2023
@suxiaogang223 suxiaogang223 marked this pull request as ready for review February 13, 2023 15:34
Copy link
Contributor

@tustvold tustvold 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 good to me, thank you.

arrow-csv/src/reader/mod.rs Outdated Show resolved Hide resolved
arrow-csv/src/reader/mod.rs Outdated Show resolved Hide resolved
arrow-csv/src/reader/mod.rs Outdated Show resolved Hide resolved
arrow-csv/src/reader/mod.rs Outdated Show resolved Hide resolved
@suxiaogang223
Copy link
Contributor Author

To support operations ArrowNativeTypeOp in generic functions, I change the code to type Native: ArrowNativeType + ArrowNativeTypeOp in privimitive_array.rs. I'm not sure if this is correct. Pay attention to this when reviewing the code🤓

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

We should probably return an error instead of panicking on overflow, but this looks good to me 👍

arrow-array/src/array/primitive_array.rs Outdated Show resolved Hide resolved
suxiaogang223 and others added 2 commits February 14, 2023 23:40
This will allow simplifying trait bounds in a number of other places

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>
@suxiaogang223
Copy link
Contributor Author

now the function return error instead of panic🤓
#3711 (review)

arrow-csv/src/reader/mod.rs Show resolved Hide resolved
arrow-csv/src/reader/mod.rs Outdated Show resolved Hide resolved
arrow-csv/src/reader/mod.rs Outdated Show resolved Hide resolved
arrow-csv/src/writer.rs Outdated Show resolved Hide resolved
@viirya viirya changed the title Feat/arrow csv decimal256 Feat: arrow csv decimal256 Feb 14, 2023
@viirya
Copy link
Member

viirya commented Feb 14, 2023

Thank you. This looks good.

@tustvold tustvold merged commit 22c1381 into apache:master Feb 14, 2023
@tustvold
Copy link
Contributor

Thank you again

@ursabot
Copy link

ursabot commented Feb 14, 2023

Benchmark runs are scheduled for baseline = d740510 and contender = 22c1381. 22c1381 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

@suxiaogang223 suxiaogang223 deleted the feat/arrow-csv-decimal256 branch February 18, 2023 04:51
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.

arrow-csv: support decimal256
4 participants