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

Add support for decimal types in ORC writer #8198

Merged
merged 40 commits into from
May 18, 2021
Merged

Add support for decimal types in ORC writer #8198

merged 40 commits into from
May 18, 2021

Conversation

vuule
Copy link
Contributor

@vuule vuule commented May 10, 2021 •
edited
Loading

Closes #8159, #7126

Current implementation uses an array to hold the exact size of each encoded element before the encode step. This allows us to simplify the encoding (each element encode is independent) and to allocate streams of exact size instead of the worst-case. The process is different from other types because decimal data streams do not use RLE encoding.

Will add benchmarks once data generator can produce decimal data.

vuule added 28 commits April 22, 2021 15:24
@vuule
placeholder comments
cd18773
@vuule
encoded size calc first impl
5b84898
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
c49f497 
…ea-orc-writer-decimal
@vuule
add per-rowgroup scan; gather rowgroup sizes
d6c0e38
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
250557b 
…ea-orc-writer-decimal
@vuule
work in progress
c62ea6e
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
04ea51c 
…ea-orc-writer-decimal
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
7e1b88c 
…ea-orc-writer-decimal
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
75b0228 
…ea-orc-writer-decimal
@vuule
use actual rowgroup size
0e93c6e
@vuule
keep decimal sizes in maps
6d89186
@vuule
aggregated column sizes; set decimal stream sizes
1995ffa
@vuule
null support in size calc
d91890a
@vuule
rename str_data_size; sink decimal sizes into encode_columns
1d090b7
@vuule
add chunk streams initialization
68321d7
@vuule
add decimal data to non_rle data size
ee2ce7c
@vuule
stats type
f6bd00b
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
9e6c250 
…ea-orc-writer-decimal
@vuule
write scale to schema
f642a60
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
4d7d68f 
…ea-orc-writer-decimal
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
7db2239 
…ea-orc-writer-decimal
@vuule
set chunk/column scale for decimal
b08228a
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
15d116d 
…ea-orc-writer-decimal
@vuule
scale encoding
f2069a9
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
88e6643 
…ea-orc-writer-decimal
@vuule
somewhat working encode
c9d8369
@vuule
Merge branch 'branch-0.20' of https://github.com/rapidsai/cudf into f…
ab58409 
…ea-orc-writer-decimal
@vuule
scale fix
9e438bd
@vuule vuule self-assigned this May 10, 2021
@vuule vuule requested review from rgsl888prabhu and devavret and removed request for ttnghia and jrhemstad May 12, 2021 23:29
isVoid
isVoid approved these changes May 13, 2021
View reviewed changes
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

pytest lgtm

rgsl888prabhu
rgsl888prabhu reviewed May 13, 2021
View reviewed changes
cpp/src/io/orc/reader_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/orc/writer_impl.cu Show resolved Hide resolved
cpp/src/io/orc/writer_impl.cu Show resolved Hide resolved
devavret
devavret suggested changes May 14, 2021
View reviewed changes
Copy link
Contributor

@devavret devavret left a comment

Choose a reason for hiding this comment

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

I wanted to say partial review but I know everything else is very orc and implementation specific.

cpp/src/io/orc/writer_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/orc/writer_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/orc/writer_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/orc/writer_impl.cu Outdated Show resolved Hide resolved
@vuule vuule requested review from devavret and rgsl888prabhu May 14, 2021 22:37
rgsl888prabhu
rgsl888prabhu reviewed May 16, 2021
View reviewed changes
Copy link
Contributor

@rgsl888prabhu rgsl888prabhu left a comment

Choose a reason for hiding this comment

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

A minor comment, apart from that looks good.

cpp/src/io/orc/stripe_enc.cu Show resolved Hide resolved
@vuule
Copy link
Contributor Author

vuule commented May 17, 2021

rerun tests

@vuule vuule changed the title Add support for decimal types in ORC writer Add support for decimal types in ORC writer May 18, 2021
@vuule vuule changed the title Add support for decimal types in ORC writer Add support for decimal types in ORC writer May 18, 2021
@vuule
Copy link
Contributor Author

vuule commented May 18, 2021

rerun tests

@vuule vuule added the 5 - Ready to Merge Testing and reviews complete, ready to merge label May 18, 2021
@vuule
Copy link
Contributor Author

vuule commented May 18, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 72e017b into rapidsai:branch-21.06 May 18, 2021
@vuule vuule deleted the fea-orc-writer-decimal branch May 18, 2021 22:38
@vuule vuule mentioned this pull request May 19, 2021
Closed
2 tasks
This was referenced Aug 10, 2021
Closed
Closed
@firestarman firestarman mentioned this pull request Sep 28, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgsl888prabhu rgsl888prabhu rgsl888prabhu approved these changes

@isVoid isVoid isVoid approved these changes

@devavret devavret devavret approved these changes

@shwina shwina Awaiting requested review from shwina

Assignees

@vuule vuule

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge cuIO cuIO issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Add support for writing dataframes containing decimal columns to orc writer
4 participants
@vuule @devavret @isVoid @rgsl888prabhu