cecemei commented on PR #19847: URL: https://github.com/apache/druid/pull/19847#issuecomment-5171346978
Thanks for the thorough review! Addressed: **[P1] Include containers from attached external files** — Confirmed: `PartialSegmentFileMapperV10`/`SegmentFileMapperV10` both track their attached external mappers privately, and neither exposes a way to get at those mappers' own `SegmentFileMetadata`. Fixing this properly needs a new accessor to aggregate "self + all externals'" containers on both mapper classes, which felt like a bigger change than this PR should carry. I've documented the limitation explicitly instead — on `QueryableIndex.getFileContainers()`, both implementations, and the `CONTAINERSIZE` analysis type javadoc — so it's a known, visible gap rather than a silent one. Filed as a follow-up rather than fixed here. **[P1] Handle indexes without a file mapper** — Rather than adding a null check for a case that (verified) never occurs on any real segment-loading path, I removed the possibility entirely: added `NoopSegmentFileMapper`, a real non-null no-op `SegmentFileMapper`, and updated the handful of tests that were passing `null` (`SimpleQueryableIndexClusteredTest`, `CompactionTaskTest`) to use it instead. `fileMapper` is now genuinely never null anywhere in the codebase, so `getFileContainers()` doesn't need to guard against it. **[P2] Preserve the existing construction ABI** — Agreed, and this was going to be a compatibility break regardless of the visibility/type changes, since a new required constructor parameter (`containers`) was added either way. Added back the old 10-arg public constructor and the old `size(int)`/`numRows(int)`/`rollup(boolean)` `Builder` overloads, marked `@Deprecated`, delegating to the new ones. **[P2] Make the new mapper method backward-compatible** — Leaving `SegmentFileMapper.getSegmentFileMetadata()` abstract intentionally. `SegmentFileMapper` isn't marked `@PublicApi`/`@ExtensionPoint` in this codebase, so it isn't a committed third-party extension surface, and the three implementations of it are the only ones that exist. Happy to reconsider if there's a known extension implementing this interface directly. -- 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]
