Core: Read content stats from v4 Manifest - #17433
Conversation
0e72e44 to
44cadbc
Compare
44cadbc to
c62305e
Compare
c62305e to
6cf0fd0
Compare
| readBuilder.setCustomType(TrackedFile.CONTENT_STATS_ID, ContentStatsStruct.class); | ||
| // content_stats holds one stats struct per projected column | ||
| for (Types.NestedField fieldStats : statsField.type().asStructType().fields()) { | ||
| readBuilder.setCustomType(fieldStats.fieldId(), FieldStatsStruct.class); |
There was a problem hiding this comment.
Does geo and variant need any special handling here? Can we add a test to make sure those types work?
There was a problem hiding this comment.
good point, I've added tests for geo + variant types with single/multiple files and this uncovered a bug around Geo types copying, which I've fixed in FieldStatsStruct
anoopj
left a comment
anoopj
left a comment
There was a problem hiding this comment.
Is the file pruning coming in a followup?
5420c62 to
ad7a0fe
Compare
d6593b4 to
4975ce5
Compare
4975ce5 to
33d52b2
Compare
| * fields referenced by the {@link #filter(Expression) filter} are always read. | ||
| */ | ||
| Builder projectStats(Iterable<Integer> fieldIds) { | ||
| Preconditions.checkArgument(fieldIds != null, "Invalid stats projection for field IDs: null"); |
There was a problem hiding this comment.
I think we have 4 modes relevant to stats so far, the default/CDC, scan planing, select column by name and project schema and we conditionally add required column depends on the filter. I am wondering if we want to add coverage for
- empty projectStats and filter (ok to use precondition to check if this combination does not make sense)
- valid projectStats and filter with default mode ( I think scan planning is already covered in
projectStatsAndFilterStatsAreCombined)
| Set<Integer> requiredFieldIds = requiredStatsProjectionForFieldIds(); | ||
| Schema fullSchema = fullSchema(requiredFieldIds); |
There was a problem hiding this comment.
nit: curious if we shall build requiredStatsType for reading stats regardless of caller's column projection ahead of time instead of passing them to calculate multiple times later? Can potentially pass requiredStatsType to fullSchema() and addRequiredColumns().
4 participants
V4ManifestReadernow reads thecontent_statscolumn, so eachTrackedFilecomesback with per-column bounds and counts.
The stats schema is derived from the table schema, so
builder()takes it as a secondargument. Stats for every column are read by default, which is what copying entries into
a new manifest needs. Callers that want less can narrow:
projectStats(fieldIds)reads stats only for the given columns,projectStats()reads noneforScanPlanning()reads only what the filter needsselect/projectomits themReading stats surfaced two bugs in the copy path, fixed here:
ContentStatsStruct.copy()threw an NPE when a projected column had no stats in themanifest
FieldStatsStructcopiedStructLikebounds by reference. Geometry and geography bounda bounding-box struct, so under
reuseContainers()every entry reported the last row'sbounds. Bounds are now deep-copied through
StructLikeUtil.copy, andStructCopyisSerializableso a copied bound still survives serialization.Used Claude for the initial prototyping but reviewed and adjusted the code manually