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

ARROW-17880: [Go] Add support for Decimal128 and Decimal256 to CSV writer #14278

Merged
merged 7 commits into from
Sep 30, 2022
Merged

ARROW-17880: [Go] Add support for Decimal128 and Decimal256 to CSV writer #14278

merged 7 commits into from
Sep 30, 2022

Conversation

mitchdevenport
Copy link
Contributor

No description provided.

@github-actions
Copy link

@mitchdevenport mitchdevenport changed the title ARROW-17880: [Go] Add support for Decimal128Type to CSV writer ARROW-17880: [Go] Add support for Decimal128 and Decimal256 to CSV writer Sep 29, 2022
go/arrow/csv/writer.go Outdated Show resolved Hide resolved
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

LGTM, when all the tests pass I'll merge it in. Thanks much!!

@zeroshade
Copy link
Member

zeroshade commented Sep 29, 2022

@mitchdevenport That said, could you create a JIRA card to add decimal128 and decimal256 to the CSV reader side? (or if you feel like it, you can incorporate it into this PR, that's up to you). It would be great to keep the types that can be written and the types that can be read in sync, but it doesn't have to be in this same change.

If you don't end up rolling it into this change, I'll try to find the time to add it to the reader after ApacheCon next week.

Thanks again!

@mitchdevenport
Copy link
Contributor Author

@mitchdevenport That said, could you create a JIRA card to add decimal128 and decimal256 to the CSV reader side? (or if you feel like it, you can incorporate it into this PR, that's up to you). It would be great to keep the types that can be written and the types that can be read in sync, but it doesn't have to be in this same change.

If you don't end up rolling it into this change, I'll try to find the time to add it to the reader after ApacheCon next week.

Thanks again!

@zeroshade Yep, no problem! Intended to make it a separate change but was unsure if the original JIRA would close on merge or not. I've updated the original ticket to be specific to the writer and have a filed a new ticket to track the reader changes: https://issues.apache.org/jira/browse/ARROW-17899

@zeroshade
Copy link
Member

Yes, this ticket will close on merge :)

@zeroshade zeroshade merged commit d77176e into apache:master Sep 30, 2022
@ursabot
Copy link

ursabot commented Sep 30, 2022

Benchmark runs are scheduled for baseline = fc9a0e7 and contender = d77176e. d77176e is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️0.61% ⬆️0.07%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.14% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] d77176e4 ec2-t3-xlarge-us-east-2
[Finished] d77176e4 test-mac-arm
[Failed] d77176e4 ursa-i9-9960x
[Finished] d77176e4 ursa-thinkcentre-m75q
[Finished] fc9a0e76 ec2-t3-xlarge-us-east-2
[Failed] fc9a0e76 test-mac-arm
[Failed] fc9a0e76 ursa-i9-9960x
[Finished] fc9a0e76 ursa-thinkcentre-m75q
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

fatemehp pushed a commit to fatemehp/arrow that referenced this pull request Oct 17, 2022
…iter (apache#14278)

Authored-by: Mitch Devenport <mitch@spice.ai>
Signed-off-by: Matt Topol <zotthewizard@gmail.com>
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.

None yet

3 participants