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
Refactor FilteredManifest and ManifestGroup #735
Conversation
d8627c9
to
e36373a
Compare
Thanks for the review @rdblue! . I addressed your comments! |
e36373a
to
4f8d017
Compare
Any thoughts on the updated PR @rdblue ? |
@rdsr, thanks for the update! I think it's close, but we need to still avoid a copy. Also, I think there is a better way to update |
6e70203
to
dd34f48
Compare
Thanks @rdsr! Just one remaining issue with the stats column projection and I think we're ready to get this in. |
} | ||
|
||
/** | ||
* @return an Iterator of DataFile. Makes defensive copies of files before returning |
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.
+1
// Make sure we have all stats columns for metrics evaluator | ||
return rowFilter != Expressions.alwaysTrue() && | ||
!columns.containsAll(ManifestReader.ALL_COLUMNS) && | ||
!columns.containsAll(STATS_COLUMNS); |
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 like how straight-forward this is now.
@rdsr, looks good to me! I'll wait until tests pass and merge it. Thanks for the quick update! |
Changes
liveEntries()
anditerator()
are now sharing underlying codeallEntries()
orliveEntries()
oriterator()
1. ManifestGroup now relies on a builderallEntries()
,liveEntries()
anditerator()
does not reuse containers and creates a copy. If this is too expensive in some case we can have a option toreuseContainers
These changes will affect #315 as well
Update
allEntries()
orliveEntries()
oriterator()
if required for metrics evaluationallEntries()
,liveEntries()
do not create copies of entries whereasiterator()
does