-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Allow Overriding AsyncFileReader used by ParquetExec #3051
Merged
Merged
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
db4e632
add metadata_ext to part file, etc
Cheappie 12dee5e
handle error
Cheappie 926d25d
fix compilation issues
Cheappie 4e9eeba
rename var
Cheappie cab87e8
fix tests compilation issues
Cheappie cecfa0a
allow user to provide their own parquet reader factory
Cheappie 124eeb9
move ThinFileReader into parquet module
Cheappie 793d252
rename field reader to delegate
Cheappie 8994e0c
convert if else to unwrap or else
Cheappie cd1e532
rename ThinFileReader to BoxedAsyncFileReader, add doc
Cheappie ef4e30e
hide ParquetFileMetrics
Cheappie cfb168f
derive debug
Cheappie 8c6d702
convert metadata_ext field into Any type
Cheappie 10c12da
add builder like method instead of modifying ctor
Cheappie e6e50c2
make `ParquetFileReaderFactory` public to let user's provide custom i…
Cheappie 6b33ea9
imports cleanup and more docs
Cheappie 2f2079a
try fix clippy failures
Cheappie cb96ca7
Disable where_clauses_object_safety
tustvold e387d23
add test
Cheappie bc66f6a
extract ParquetFileReaderFactory test to integration tests
Cheappie 334edce
resolve conflicts
Cheappie 30d664a
further cleanup
Cheappie 9c2ccc3
Merge pull request #1 from tustvold/fix-send
Cheappie e18d26f
fix: Add apache RAT license
alamb cec773f
fix send
Cheappie File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
❤️
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 test looks great
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 have moved that test to integration tests, but for users convenience I had to expose two things
ParquetFileMetrics
andfn fetch_parquet_metadata(...)
. I have added docs that describe that these things are subjects to change in near future and are exposed for low level integrations. Should we worry about exposing impl details, or having such doc in place that these things might change is enough ?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 @Cheappie -- I think the comments are good enough
In fact it is perhaps good to see what was required to be made
pub
to use these new APIs. Hopefully it makes downstream integrations easier