FrankChen021 commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4348259217
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -1183,13 +1183,33 @@ static class ExistingSegmentAnalyzer
* on finding any 'range' partition dimension to be multivalued at
runtime, which ends up in a mismatch between
* the compaction config and the actual segments (lastCompactionState),
leading to repeated compactions.
* <p>
- * Note that needProjections alone does not trigger a download: every
caller that asks for projections also asks
- * for something above, so the lists stay in step. A caller that wants
only projections would have to add it here.
+ * Projections are only considered a reason to analyze if we can confirm
that any existing segments contain them via
+ * {@link #anySegmentHasProjections()}.
*/
private boolean shouldDownloadSegments()
{
+ return needRollup
+ || needQueryGranularity
+ || needDimensionsSpec
+ || needMetricsSpec
+ || needMultiValuedDimensions
+ || (needProjections && anySegmentHasProjections());
+ }
- return needRollup || needQueryGranularity || needDimensionsSpec ||
needMetricsSpec || needMultiValuedDimensions;
+ /**
+ * Whether any segment to be compacted stores a projection, taken from
{@link DataSegment#getProjections()} so
+ * that it costs no I/O. A null list there means a segment written before
Druid had projections, which therefore
+ * has none.
+ */
+ private boolean anySegmentHasProjections()
+ {
+ for (Pair<DataSegment, Supplier<ResourceHolder<QueryableIndex>>>
segmentPair : segmentsIterable) {
+ final List<String> projectionNames = segmentPair.lhs.getProjections();
Review Comment:
[P1] Projection metadata is unavailable in the production path
**Finding:** SegmentProvider.findSegments() requests only
SegmentDetail.LOAD_SPEC, and RetrieveUsedSegmentsAction nulls unrequested
fields including projections. Consequently, anySegmentHasProjections() returns
false for normal compaction tasks and existing projections are still omitted.
**Suggestion:** Request SegmentDetail.PROJECTIONS when retrieving segments
used for schema analysis, and cover the end-to-end compaction path.
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/task/CompactionTask.java:
##########
@@ -1183,13 +1183,33 @@ static class ExistingSegmentAnalyzer
* on finding any 'range' partition dimension to be multivalued at
runtime, which ends up in a mismatch between
* the compaction config and the actual segments (lastCompactionState),
leading to repeated compactions.
* <p>
- * Note that needProjections alone does not trigger a download: every
caller that asks for projections also asks
- * for something above, so the lists stay in step. A caller that wants
only projections would have to add it here.
+ * Projections are only considered a reason to analyze if we can confirm
that any existing segments contain them via
+ * {@link #anySegmentHasProjections()}.
*/
private boolean shouldDownloadSegments()
{
+ return needRollup
+ || needQueryGranularity
+ || needDimensionsSpec
+ || needMetricsSpec
+ || needMultiValuedDimensions
+ || (needProjections && anySegmentHasProjections());
+ }
- return needRollup || needQueryGranularity || needDimensionsSpec ||
needMetricsSpec || needMultiValuedDimensions;
+ /**
+ * Whether any segment to be compacted stores a projection, taken from
{@link DataSegment#getProjections()} so
+ * that it costs no I/O. A null list there means a segment written before
Druid had projections, which therefore
+ * has none.
+ */
+ private boolean anySegmentHasProjections()
+ {
+ for (Pair<DataSegment, Supplier<ResourceHolder<QueryableIndex>>>
segmentPair : segmentsIterable) {
+ final List<String> projectionNames = segmentPair.lhs.getProjections();
+ if (projectionNames != null && !projectionNames.isEmpty()) {
Review Comment:
[P1] Null projection metadata is treated as absence
**Finding:** DataSegment.getProjections() == null means the segment is
unaware of this optional field, not necessarily that its index has no
projections. Projection-capable segments created before this metadata field was
added can therefore be skipped and rewritten without their existing projections.
**Suggestion:** Treat null metadata conservatively as requiring projection
analysis, or obtain projection metadata directly from the segment index, with
coverage for legacy segments.
--
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]
