feat: Add CONTAINERSIZE segment metadata analysis type - #19847
Conversation
Adds SegmentMetadataQuery.AnalysisType.CONTAINERSIZE, which reports each V10 segment file container's owning bundle name and on-disk byte size via SegmentAnalysis.getContainers(). Only populated for segments written in the V10 file format; pre-V10 segments report null, consistent with how other analysis types handle segments that predate them. - QueryableIndex.getFileContainers() (default null) sources this from SegmentFileMapper.getSegmentFileMetadata(), implemented by SimpleQueryableIndex/PartialQueryableIndex; SmooshedFileMapper (legacy) explicitly returns null since it has no container/bundle structure. - Merging sums sizes by bundle name rather than concatenating raw per-segment container lists, since a container has no identity across segments; single-sided merges pass the non-null side through unchanged. - SegmentAnalysis.ContainerAnalysis is nested (not top-level) since it has no identity outside that one field, and SegmentAnalysis.Builder gained bulk setters so production call sites don't need the raw constructor. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Removes a redundant parenthetical and condenses the explanation of why one-sided merges return unchanged rather than passing through the per-bundle collapse. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FrankChen021
left a comment
FrankChen021
left a comment
There was a problem hiding this comment.
| Severity | Findings |
|---|---|
| P0 | 0 |
| P1 | 2 |
| P2 | 2 |
| P3 | 0 |
| Total | 4 |
Reviewed 19 of 19 changed files.
This is an automated review by Codex GPT-5.6-Sol
| @Override | ||
| public List<SegmentFileContainerMetadata> getFileContainers() | ||
| { | ||
| return metadata.getContainers(); |
There was a problem hiding this comment.
[P1] Include containers from attached external files
This returns only the entry-point metadata's containers. PartialSegmentFileMapperV10 may have attached external mappers, and its own documentation notes that a bundle can span the main file and externals. Any data written through SegmentFileBuilder.getExternalBuilder is therefore silently omitted from CONTAINERSIZE, underreporting projection storage. The eager SimpleQueryableIndex path has the same limitation through SegmentFileMapperV10.getSegmentFileMetadata(). Aggregate containers from the main mapper and every attached external mapper, with coverage for an external-backed bundle.
| @Nullable | ||
| public List<SegmentFileContainerMetadata> getFileContainers() | ||
| { | ||
| final SegmentFileMetadata segmentFileMetadata = fileMapper.getSegmentFileMetadata(); |
There was a problem hiding this comment.
[P1] Handle indexes without a file mapper
SimpleQueryableIndex permits a null fileMapper—existing constructors and tests use this for in-memory indexes, and close() already guards it—but this new method dereferences it unconditionally. A CONTAINERSIZE query against such a valid index now fails with an NPE instead of returning the documented unsupported/null result. Check fileMapper before calling getSegmentFileMetadata().
|
|
||
| @JsonCreator | ||
| public SegmentAnalysis( | ||
| SegmentAnalysis( |
There was a problem hiding this comment.
[P2] Preserve the existing construction ABI
The previous public ten-argument constructor is replaced by an eleven-argument package-private constructor. The PR also changes the JVM signatures of the public size(int), numRows(int), and rollup(boolean) builder methods. Existing compiled extensions constructing SegmentAnalysis will encounter NoSuchMethodError, while source consumers outside this package can no longer call the constructor. Retain deprecated delegating overloads for the old constructor and primitive builder signatures.
| * if unsupported (e.g. legacy pre-V10 mappers that don't track this structure). | ||
| */ | ||
| @Nullable | ||
| SegmentFileMetadata getSegmentFileMetadata(); |
There was a problem hiding this comment.
[P2] Make the new mapper method backward-compatible
Adding getSegmentFileMetadata() as an abstract interface method means existing third-party mapper implementations lack it and can throw AbstractMethodError when this analysis is requested. Since unsupported mappers are explicitly supposed to return null, make this a default method returning null; the V10 implementations can continue overriding it.
2 participants
Description
Adds a new
SegmentMetadataQueryanalysis type,CONTAINERSIZE, that reports the on-disk byte size of each segment's internal storage containers, broken down by the base table and each aggregate projection. This lets a user (or an internal tool) inspect how much storage an individual aggregate projection is adding to a segment, on top of the base table it's derived from.When
CONTAINERSIZEis requested,SegmentAnalysis.getContainers()returns a list of(bundle, size)pairs, one per physical storage container in the segment's V10 file, wherebundleis either the base table or a projection's name. This is only available for segments written in the V10 segment file format; segments written in the (currently default) V9/legacy format reportnullhere.A few implementation notes:
QueryableIndex.getFileContainers()is the new extension point sourcing this data, implemented bySimpleQueryableIndex/PartialQueryableIndexviaSegmentFileMapper.getSegmentFileMetadata();SmooshedFileMapper(legacy) explicitly returnsnullsince the legacy format has no container/bundle structure to report.Release note
Added a new
CONTAINERSIZEsegment metadata analysis type that reports the on-disk byte size of each segment's storage containers, broken down by the base table and each aggregate projection. This makes it possible to see how much storage an individual projection is adding to a segment. Only available for segments written in the V10 segment file format.Key changed/added classes in this PR
SegmentMetadataQuerySegmentAnalysisSegmentAnalysis.ContainerAnalysisSegmentMetadataQueryRunnerFactorySegmentMetadataQueryQueryToolChestQueryableIndexThis PR has: