-
Notifications
You must be signed in to change notification settings - Fork 865
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
Add in JNI for parsing JSON data and getting the metadata back too. #11431
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.
This looks good to me.
Codecov Report
@@ Coverage Diff @@
## branch-22.10 #11431 +/- ##
===============================================
Coverage ? 86.47%
===============================================
Files ? 144
Lines ? 22856
Branches ? 0
===============================================
Hits ? 19764
Misses ? 3092
Partials ? 0 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
* A table along with some metadata about the table. This is typically returned when | ||
* reading data from an input file where the metadata can be important. | ||
*/ | ||
public class TableWithMeta implements AutoCloseable { |
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.
Can this be implemented as a derived class extending Table
?
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.
It would not match what the C++ code is doing. If you want me to try I can. I agree that conceptually it would be more interesting, but I wanted to preserve the C++ API in this case. Also it gets to be a more invasive change to make it work.
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 see. So it is just some wrapper for special purposes.
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.
Yes. For all of the other APIs where we read the from a file we already know the names of the columns, as we passed them into the reader. So in those cases we just return a Table with the columns in the order that was requested when it was passed in. Here we need the metadata to be able to know what the names of the columns are. Long term we should probably update all of our reader APIs to match what CUDF is doing in C++ and just return the TableWithMeta
. But for now to minimize the impact of the change we decided not to do that. I do like the idea of having TableWithMeta
be a Table
too. We probably will/should implement that if we do try to update all of the other APIs, just because it would minimize the impact of the change on customer code.
Some Python tests failed. Let's try to merge upstream. |
4dee804
to
add02d2
Compare
@gpucibot merge |
Description
Adds in a new java binding to allow reading a JSON buffer and getting back the metadata along with the table when inferring the schema.
Checklist