-
Notifications
You must be signed in to change notification settings - Fork 915
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
move filepath and mmap logic out of json/csv up to functions.cpp #9040
move filepath and mmap logic out of json/csv up to functions.cpp #9040
Conversation
rerun tests |
rerun tests |
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #9040 +/- ##
===============================================
Coverage ? 10.73%
===============================================
Files ? 114
Lines ? 19060
Branches ? 0
===============================================
Hits ? 2046
Misses ? 17014
Partials ? 0 Continue to review full report at Codecov.
|
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.
Good stuff. We're finally done passing file names to the readers 👍
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.
LGTM!
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.
Very informative read, thanks for doing this.
@gpucibot merge |
Depends on #9040 Authors: - Christopher Harris (https://github.com/cwharris) Approvers: - Nghia Truong (https://github.com/ttnghia) - Karthikeyan (https://github.com/karthikeyann) URL: #9090
Depends on #9040 Most of this is just rearranging code and renaming some variables. The reason I did this is because having all of the reader functionality as individual functions means more localized cognitive loads. Each function declares exactly what it needs to do it's job, and can enforce const-ness on a more granular level. This makes reasoning about the code easier. ~It turns out this refactor revealed that the filename-associated logic within the csv reader impl was never being used. The filename is always an empty string, and calling the infer_compression_type function was just a way to stringify the compression type passed in by the reader options.~ Correction, there was some filename related logic being used, but I've factored that out in PR #9040. The changes made in #9040 also allow us to pass only a single datasource to `read_csv`, as that is all that is supported. Therefore the error handling related to passing multiple datasources can be moved upstream. This PR enables many more simplifications to be made to csv, but I'd like to stop here with this PR, because so far it is purely organizational changes. Any more simplifications will involving refactoring individual functions, which would make this PR incredibly difficult to review. Authors: - Christopher Harris (https://github.com/cwharris) Approvers: - Vukasin Milovanovic (https://github.com/vuule) - Jordan Jacobelli (https://github.com/Ethyling) URL: #9041
Depends on #9040 Removes the json reader and impl classes, replacing member variables with local variables, reduces cognitive overhead, and facilitates further refactoring. Authors: - Christopher Harris (https://github.com/cwharris) Approvers: - Ram (Ramakrishna Prabhu) (https://github.com/rgsl888prabhu) - MithunR (https://github.com/mythrocks) - Elias Stehle (https://github.com/elstehle) URL: #9088
Depends on #9040 and (unfortunately) #9041 Authors: - Christopher Harris (https://github.com/cwharris) Approvers: - Robert Maynard (https://github.com/robertmaynard) - Vukasin Milovanovic (https://github.com/vuule) URL: #9089
Removes the filepath-related logic from readers, moving whole-file compression type inference up to
io/functions.cpp
. Also moves the lazy mmap datasource creation logic out csv/json reader and up toio/functions.cpp
.