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.
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
Implement row group skipping for the default engine parquet readers #362
Implement row group skipping for the default engine parquet readers #362
Changes from 31 commits
715f233
ef71f1a
39b8927
b5c3a52
e71571e
cbca3b3
e7d87eb
beeb6e8
519acbd
18b33cf
6c98441
0fdaf0a
8ac33f8
1cf03dc
bc8b344
0971002
375a380
6236874
9efcbf7
46d19e3
7666512
f3865d0
a4dc3da
40131db
bf65904
cce762d
c7d6bb0
4f92ed7
08a305b
e8a947e
bf1e3a8
9d632e7
4a77f3a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
we could define an enum for this rather than
Some(None)
. Just a thought, I'm okay with both ways, and theSome(None)
will have cleaner code in the method (at the cost of a more confusing return type)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.
oh nice i think i commented on this too - maybe just Result? and we can have
Err(MissingColumn)
for a more understandable return type?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 avoided
Result
because that would be exceptions as control flow (this isn't actually an error, it's just a situation).If this were public code, the enum might make sense. But for a private method, I don't think it's worth the cognitive overhead (both to define and use it) when one line of code comment can fully explain what's going on?
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.
Yeah, I think it's fine as is since both the method and the call site have a comment.
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.
sgtm!