-
Notifications
You must be signed in to change notification settings - Fork 894
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
Remove protobuf and use parsed ORC statistics from libcudf #15564
Remove protobuf and use parsed ORC statistics from libcudf #15564
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few TODO notes for myself on what I see as the weakest points of this PR. @vyasr You may have feedback on these TODOs since they're Cython related. I proposed some next steps, let me know if they sound reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as CMake codeowner (only CMake change is deletion of a file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving C++ changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this Bradley! I have a couple of small suggestions/questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm heading out for a bit and I know that you want to get this in before you leave for vacation, so I'm going to go ahead and approve. It looks like all of my suggestions have been addressed other than the list comprehensions, which is just a cleanup task. Please do it if you can, but I trust that whatever version you end up with will be good enough. Thanks for this PR! Removing protobuf like this will be great.
/merge |
Description
This PR removes the cuDF Python dependencies on
protobuf
andprotoc-wheel
. Closes #15511.The only use case for the
protobuf
dependency was reading ORC file/stripe statistics. However, we have code in libcudf that can do this without requiringprotobuf
.In this PR, we expose the C++ code for parsing ORC statistics from libcudf to Cython and remove all references to
protobuf
.Checklist