Add projection data to SegmentAnalysis#18119
Conversation
| * @param projections2 second map of projections to merge | ||
| * @return merged map of projections | ||
| */ | ||
| public static Map<String, AggregateProjectionMetadata> merge( |
There was a problem hiding this comment.
I think this logic should be part of the segment metadata query stuff instead of living here since it is specific to that query, so it isn't unintentionally used for other purposes. Similarly, the Metadata class has its own logic that it uses when combining the Metadata during segment creation, though with different logic that is validating the projections are all the same: https://github.com/apache/druid/blob/master/processing/src/main/java/org/apache/druid/segment/Metadata.java#L344
There was a problem hiding this comment.
moved it out to SegmentMetadataQueryQueryToolChest.
| QUERYGRANULARITY, | ||
| ROLLUP; | ||
| ROLLUP, | ||
| PROJECTIONS; |
There was a problem hiding this comment.
will need to update the docs to include this new analysis type too at some point, https://github.com/apache/druid/blob/master/docs/querying/segmentmetadataquery.md#analysistypes, and also include the fact that when merging results that any mismatched projections will be omitted. It would be fine to do this as a follow-up or part of this PR is fine too
| /** | ||
| * Helper class to build {@link SegmentAnalysis} objects for testing purposes. | ||
| */ | ||
| public class SegmentAnalysisBuilder |
There was a problem hiding this comment.
nit: we commonly put builders inside of the class they build as an internal class just named like Builder, which i think helps with visibility, so people already looking at a class know it has a builder, and just have a static method like SegmentAnalysis.builder().addFoo(..).build()
There was a problem hiding this comment.
I moved it to SegmentAnalysis. I put it in test folder initially because it looks like it's mostly needed in tests, not in production code, but i guess it's no harm to put it as internal static class as well. It might also help to unify when we use null, v.s. empty collections.
Description
New Feature
SegmentAnalysisshould returnAggregateProjectionMetadatain the segment ifAnalysisType.PROJECTIONSis included inSegmentMetadataQuery. If schema doesn't match for the same projection, the specific schema won't be included in the result.Testing & Refactor
SegmentAnalysisBuilderclass.SegmentMetadataQueryQueryToolChestTest.IndexMergerV9, movesortOrdermust include time column check in projections out ofmakeIndexFiles, to its call site. Also,Metadata.merge(metadataList, combiningMetricAggs)would never take null inputs, this refactor makes it more obvious.SegmentMetadataQueryQueryToolChest.mergeAnalyses, throw defensive exception if no data source in the query.Key changed/added classes in this PR
SegmentMetadataQuerySegmentAnalysisSegmentMetadataQueryQueryToolChestTestIndexMergerV9This PR has: