Core: Add all_delete_files and all_files tables#4694
Conversation
|
FYI @RussellSpitzer @aokolnychyi if you guys have some cycles, thanks |
| return Sets.newHashSet(MetadataTableType.ALL_DATA_FILES, | ||
| MetadataTableType.ALL_DATA_FILES, | ||
| MetadataTableType.ALL_FILES) |
There was a problem hiding this comment.
Nit: Consider making this a static final constant.
| } | ||
| } | ||
|
|
||
| private boolean allFileTableTest(MetadataTableType tableType) { |
There was a problem hiding this comment.
Nit: This might read better as isAllFilesTable, though I recognize there is an AllFilesTable now.
Maybe isOneOfAllFilesType or isTableAggregatedFromAllSnapshots?
Given there will be an ALL_FILES table, I'm not sure of the best name to add for this but throwing those out there.
There was a problem hiding this comment.
What about isAggFileTable?
|
Let me take a look at this one as well. |
aokolnychyi
left a comment
aokolnychyi
left a comment
There was a problem hiding this comment.
Looks correct to me. Spotted a few typos in names.
| import org.apache.iceberg.io.CloseableIterable; | ||
|
|
||
| /** | ||
| * A {@link Table} implementation that exposes a table's valid delete files as rows. |
There was a problem hiding this comment.
nit: redundant a before table's valid delete files?
There was a problem hiding this comment.
Hm, I'm not sure about if its redundant ( 'table' needs an article, right?). How about:
A table implementation that exposes its valid delete files as rows
| return MetadataTableType.ALL_DELETE_FILES; | ||
| } | ||
|
|
||
| public static class AllDataFilesTableScan extends BaseAllFilesTableScan { |
There was a problem hiding this comment.
Typo: should be AllDeleteFilesTableScan
| import org.apache.iceberg.io.CloseableIterable; | ||
|
|
||
| /** | ||
| * A {@link Table} implementation that exposes a table's valid files as rows. |
| return MetadataTableType.ALL_FILES; | ||
| } | ||
|
|
||
| public static class AllDataFilesTableScan extends BaseAllFilesTableScan { |
There was a problem hiding this comment.
Typo in the name as well.
| return Sets.newHashSet(MetadataTableType.ALL_DATA_FILES, | ||
| MetadataTableType.ALL_DATA_FILES, | ||
| MetadataTableType.ALL_FILES) |
| } | ||
| } | ||
|
|
||
| private boolean allFileTableTest(MetadataTableType tableType) { |
There was a problem hiding this comment.
What about isAggFileTable?
|
Thanks @aokolnychyi and @kbendick for review |
3 participants
This will be the final set of changes for #4139, adding the new metadata tables 'all_delete_files' and 'all_files' to include delete files.
This adds both in one change list as it's easier to write a unified test, although they can be split up if we prefer that.