FrankChen021 commented on code in PR #19847:
URL: https://github.com/apache/druid/pull/19847#discussion_r3712424272
##########
processing/src/main/java/org/apache/druid/segment/SimpleQueryableIndex.java:
##########
@@ -333,12 +336,23 @@ public SegmentFileMapper getFileMapper()
return fileMapper;
}
+ /**
+ * Doesn't include containers from any external mapper {@link #fileMapper}
may have attached (see
+ * {@link SegmentFileMapperV10}) — those aren't reflected in {@link
SegmentFileMetadata#getContainers()} of the
+ * entry-point mapper, so a bundle whose data spilled into an external file
will be undercounted here.
+ */
+ @Override
+ @Nullable
+ public List<SegmentFileContainerMetadata> getFileContainers()
+ {
+ final SegmentFileMetadata segmentFileMetadata =
fileMapper.getSegmentFileMetadata();
Review Comment:
[P1] Include containers from attached external files
getSegmentFileMetadata() only returns the entry-point file's metadata, while
V10 mappers may attach external segment files containing additional containers.
Consequently, CONTAINERSIZE systematically understates disk usage for spilled
or multi-file bundles. Documenting the limitation does not make the reported
total correct; aggregate containers from the entry point and every attached
mapper. The partial mapper already exposes getExternalFilenames() and
getExternalMapper() for this traversal.
##########
processing/src/main/java/org/apache/druid/segment/file/SegmentFileMapper.java:
##########
@@ -61,6 +61,13 @@ default ByteBuffer mapExternalFile(String filename, String
name) throws IOExcept
return mapFile(name);
}
+ /**
+ * Returns the {@link SegmentFileMetadata} describing this mapper's
containers and internal files, or {@code null}
+ * if unsupported (e.g. legacy pre-V10 mappers that don't track this
structure).
+ */
+ @Nullable
+ SegmentFileMetadata getSegmentFileMetadata();
Review Comment:
[P2] Keep the mapper metadata method binary-compatible
Adding this abstract method breaks precompiled implementations of the public
SegmentFileMapper interface: invoking SimpleQueryableIndex.getFileContainers()
on one throws AbstractMethodError. Since unsupported mappers are explicitly
represented by null, make this a default method returning null; built-in V10
implementations can continue overriding it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
