FrankChen021 commented on code in PR #19847:
URL: https://github.com/apache/druid/pull/19847#discussion_r3695809964
##########
processing/src/main/java/org/apache/druid/query/metadata/metadata/SegmentAnalysis.java:
##########
@@ -60,9 +61,10 @@ public class SegmentAnalysis implements
Comparable<SegmentAnalysis>
private final TimestampSpec timestampSpec;
private final Granularity queryGranularity;
private final Boolean rollup;
+ private final List<ContainerAnalysis> containers;
@JsonCreator
- public SegmentAnalysis(
+ SegmentAnalysis(
Review 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.
##########
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] 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.
##########
processing/src/main/java/org/apache/druid/segment/SimpleQueryableIndex.java:
##########
@@ -333,6 +335,14 @@ public SegmentFileMapper getFileMapper()
return fileMapper;
}
+ @Override
+ @Nullable
+ public List<SegmentFileContainerMetadata> getFileContainers()
+ {
+ final SegmentFileMetadata segmentFileMetadata =
fileMapper.getSegmentFileMetadata();
Review 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().
##########
processing/src/main/java/org/apache/druid/segment/PartialQueryableIndex.java:
##########
@@ -298,6 +299,13 @@ public List<OrderBy> getOrdering()
return ordering;
}
+ @Nullable
+ @Override
+ public List<SegmentFileContainerMetadata> getFileContainers()
+ {
+ return metadata.getContainers();
Review 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.
--
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]
