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

[REVIEW] Enable proper Index round-tripping in orc reader and writer #10170

Merged
merged 7 commits into from
Feb 24, 2022
Merged

[REVIEW] Enable proper Index round-tripping in orc reader and writer #10170

merged 7 commits into from
Feb 24, 2022

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Jan 31, 2022 •
edited
Loading

Fixes: #10010

This PR:

  • Fixes to_orc by enabling writing of dataframe metadata to ORC file being created.
  • Fixes read_orc to correctly read and assign Index objects that exist in metadata
  • Note: This change is not backward compatible with files already written in previous versions of cudf.

@galipremsagar galipremsagar added bug Something isn't working Python Affects Python cuDF API. 4 - Needs cuDF (Python) Reviewer cuIO cuIO issue breaking Breaking change labels Jan 31, 2022
@galipremsagar galipremsagar self-assigned this Jan 31, 2022
@galipremsagar galipremsagar requested a review from a team as a code owner January 31, 2022 16:19
@codecov
Copy link

codecov bot commented Jan 31, 2022 •
edited
Loading

Codecov Report

Merging #10170 (7441ff5) into branch-22.04 (a7d88cd) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10170      +/-   ##
================================================
+ Coverage         10.42%   10.62%   +0.19%     
================================================
  Files               119      122       +3     
  Lines             20603    20977     +374     
================================================
+ Hits               2148     2228      +80     
- Misses            18455    18749     +294
Impacted Files Coverage Δ
...ython/custreamz/custreamz/tests/test_dataframes.py 99.39% <0.00%> (-0.01%) ⬇️
python/cudf/cudf/errors.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/orc.py 0.00% <0.00%> (ø)
python/cudf/cudf/_version.py 0.00% <0.00%> (ø)
python/cudf/cudf/datasets.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <0.00%> (ø)
python/cudf/cudf/io/parquet.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/series.py 0.00% <0.00%> (ø)
python/cudf/cudf/utils/utils.py 0.00% <0.00%> (ø)
... and 35 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 496f452...7441ff5. Read the comment docs.

@vuule
Copy link
Contributor

vuule commented Feb 1, 2022

This change is not backward compatible with files already written in previous versions of cudf.

How come? Are the old files invalid in some way (with respect to ORC specs)?

@galipremsagar
Copy link
Contributor Author

galipremsagar commented Feb 1, 2022 •
edited
Loading

How come? Are the old files invalid in some way (with respect to ORC specs)?

Because there is no metadata being written previously. The only way we can correctly "know" the index column is by writing it in metadata and reading the same metadata.

The old files will not be invalid, they will still be read in the old way i.e., the index column will become a column of the actual dataframe rather than the index of the dataframe.

vuule
vuule reviewed Feb 1, 2022
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

optional suggestions

python/cudf/cudf/_lib/orc.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/orc.pyx Outdated
@@ -123,8 +129,59 @@ cpdef read_orc(object filepaths_or_buffers,
c_result = move(libcudf_read_orc(c_orc_reader_options))

names = [name.decode() for name in c_result.metadata.column_names]
cdef map[string, string] user_data = c_result.metadata.user_data
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be good to place this code in a separate function, not sure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies for the delay, moved this to a separate function.

rgsl888prabhu
rgsl888prabhu reviewed Feb 24, 2022
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.

Just have small question

python/cudf/cudf/_lib/orc.pyx Show resolved Hide resolved
python/cudf/cudf/_lib/orc.pyx Show resolved Hide resolved
@galipremsagar
Copy link
Contributor Author

Thanks for reviewing this @rgsl888prabhu !

@galipremsagar galipremsagar added the 5 - Ready to Merge Testing and reviews complete, ready to merge label Feb 24, 2022
@galipremsagar
Copy link
Contributor Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 3a1dbe8 into rapidsai:branch-22.04 Feb 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuule vuule vuule left review comments

@rgsl888prabhu rgsl888prabhu rgsl888prabhu approved these changes

@brandon-b-miller brandon-b-miller Awaiting requested review from brandon-b-miller brandon-b-miller is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees

@galipremsagar galipremsagar

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change bug Something isn't working cuIO cuIO issue Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Name of Columns being Shifted when Written
3 participants
@galipremsagar @vuule @rgsl888prabhu