Core: Implement stats filtering in V4ManifestReader - #18147
Conversation
| protected boolean shouldKeep(E item) { | ||
| boolean matches = pred.test(item); | ||
| if (!matches) { | ||
| skipCallback.accept(item); |
There was a problem hiding this comment.
This variant of filter uses a callback that is passed the item that is skipped so that we can update the right counter depending on the item. In this case, update the manifest or data file counter based on the content type.
This iterator is identical to the one removed below, except that it calls skipCallback.accept instead of counter.increment.
c7a450a to
1641c82
Compare
|
|
||
| if (!includeAll) { | ||
| entries = CloseableIterable.filter(entries, entry -> entry.tracking().isLive()); | ||
| files = CloseableIterable.filter(files, file -> file.tracking().isLive()); |
There was a problem hiding this comment.
I reordered this so that the cheapest filter is evaluated first. That way more expensive stats or partition filters are not run for files that are skipped because of status.
| } else { | ||
| files = | ||
| CloseableIterable.filter( | ||
| this::incrementSkipCount, files, file -> file.recordCount() != 0L); |
There was a problem hiding this comment.
This branch skips files based on record count when there is no row filter.
| } | ||
|
|
||
| private static TrackedFile unpartitionedDataWithDVFile(String location, String dvLocation) { | ||
| return new TrackedFileStruct( |
There was a problem hiding this comment.
Refactored to share an implementation with other helpers.
|
|
||
| @ParameterizedTest | ||
| @FieldSource("MANIFEST_FORMATS") | ||
| public void statsFilterCaseSensitivity(FileFormat format) throws IOException { |
There was a problem hiding this comment.
nit: should we call this statsFilterCaseInsensitive?
should we also add a statsFilterCaseSensitive where the upper case filter doesn't skip FILE_C
There was a problem hiding this comment.
This tests both case sensitive and case insensitive. First it verifies that a filter that requires case sensitivity fails and then it turns on case sensitivity and verifies the filter works.
I think the confusion is that when case doesn't match, the result is ValidationException because the filter cannot be bound to the schema, rather than just ignoring the filter.
|
Merged. Thanks, @stevenzwu! |
Reconcile the partition projection with main's V4ManifestReader rework (apache#18109 content stats, apache#18147 stats filtering, apache#18138 ManifestBitmap MDV)
2 participants
This implements stats filtering in
V4ManifestReaderusing anInclusiveStatsFilter.This required updating
CloseableIterable.filterto be able to pass a callback for updating metrics based on a skipped item. A reasonable follow up would be to rewrite the partition filtering to use this, but #18108 is changing partition handling so I thought it would be better to wait and avoid conflicts.Test plan:
InclusiveStatsFilteris thoroughly tested, so it should not be exhaustively tested hereInclusiveStatsFilter.evalare passed correctly (content stats and record count)forScanPlanningthat projects no stats by defaultprojectandselectthat do not request stats columnsI also added a test for bucket partition skipping, which will eventually move to stats-based skipping. This currently passes based on partition filtering.