You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "clintropolis (via GitHub)" <gi...@apache.org> on 2026/10/22 06:25:46 UTC
[PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
clintropolis opened a new pull request, #20684:
URL: https://github.com/apache/druid/pull/20684
### Description
Fixes an issue that could be caused by `CompactionTask.ExistingSegmentAnalyzer.shouldDownloadSegments()` not including `needProjections` in the check, so a config that declares dimensions, metrics, rollup, etc, but not projections could incorrectly remove projections from existing segments. To fix, `needProjections` is now considered, but also combined with a check for any projection names from `DataSegment.getProjections()` so any non-empty lists can be used to skip analyzing if there are definitely no existing projections to be found.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "clintropolis (via GitHub) via commits" <co...@druid.apache.org>.
clintropolis commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4429585931
##########
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:
prior to this patch, these projections were _always_ removed from all segments (even those created after the method was added) unless some other path triggered segment analysis because we didn't check if we needed to check for projections at all as an input to 'should we analyze'. Even after this patch, if any other path still triggers the need for analysis (omitted dimension/metrics/etc), then we pick up those legacy projections from segments created between 31 and 34, so that behavior is the same as before. This patch just makes it so that if the inline compaction config doesn't specify any projections that that alone can trigger analysis if we detect any existing projections via the DataSegment method. So the behavior is no worse than the existing behavior for segments created in that window, and an improvement for all segments created after 34 with projections.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "capistrant (via GitHub) via commits" <co...@druid.apache.org>.
capistrant commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4443954643
##########
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:
I am inclined to support Clint's current implementation over the alternative of always analyzing segments when null comes back. If there is a path to some type of middle ground where we could reconcile this for the data owner quietly without causing massive amounts of extra work for all the cases where there really isn't projections under the hood, I'd consider it. But a detailed upgrade notice/warning could be good enough IMO.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "capistrant (via GitHub) via commits" <co...@druid.apache.org>.
capistrant commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4443954643
##########
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:
I am inclined to support Clint's current implementation over the alternative of always analyzing segments when null comes back. If there is a path to some type of middle ground where we could reconcile this for the data owner quietly without causing massive amounts of extra work for all the cases where there really isn't projections under the hood, I'd consider it. But a detailed upgrade notes notice/warning could be good enough IMO.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "FrankChen021 (via GitHub) via commits" <co...@druid.apache.org>.
FrankChen021 commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4431669650
##########
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:
I reviewed all 4 changed files and the surrounding DataSegment and retrieval code. I agree this does not worsen the pre-existing behavior for legacy segments, but that does not close the P1 for this fix: DataSegment explicitly defines null as not aware of projections, while an empty list means projection-aware with none. With a fully specified inline compaction, a datasource whose legacy segments all have null metadata makes shouldDownloadSegments() return false, so no index metadata is read and any projections still in those indexes are omitted from the replacement schema. Most likely already removed is not a trusted invariant; such segments can exist without a prior compaction. Please either treat null as unknown, establish a trusted compatibility boundary, or inspect the index, and add a regression test for the null/legacy case. Reviewed 4 of 4 changed files.
<!-- mergelens:review -->
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "clintropolis (via GitHub) via commits" <co...@druid.apache.org>.
clintropolis commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4429585931
##########
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:
prior to this patch, these projections were _always_ removed from all segments unless some other path triggered segment analysis because we didn't check if we needed to check for projections at all as an input to 'should we analyze'. Even after this patch, if any other path still triggers the need for analysis (omitted dimension/metrics/etc), then we pick up those legacy projections from segments created between 31 and 34, so that behavior is the same as before. This patch just makes it so that if the inline compaction config doesn't specify any projections that that alone can trigger analysis if we detect any existing projections via the DataSegment method. So the behavior is no worse than the existing behavior for segments created in that window, and an improvement for all segments created after 34 with projections.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "FrankChen021 (via GitHub) via commits" <co...@druid.apache.org>.
FrankChen021 commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4429651111
##########
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:
Reviewed 4 of 4 changed files for this follow-up. Thanks for clarifying the performance trade-off. The added `SegmentDetail.PROJECTIONS` request fixes the metadata-aware case, but the original P1 remains open for legacy segments: `DataSegment.getProjections() == null` means the segment is not projection-aware, not that its index has no projections. Projection support predates this field, so a fully specified compaction can still skip loading such a segment and rewrite it without its projections. Please treat null as unknown (or establish a trusted compatibility boundary / inspect the index) and add a regression test for that case.
<!-- mergelens:review -->
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "clintropolis (via GitHub) via commits" <co...@druid.apache.org>.
clintropolis commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4429806674
##########
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:
i'm not convinced to change this for the reason stated in my previous comment, as most likely these projections have already been removed if compaction is enabled.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "FrankChen021 (via GitHub) via commits" <co...@druid.apache.org>.
FrankChen021 commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4454844020
##########
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:
The latest clarification does not close this finding. I rechecked the current head and all four changed files. `DataSegment` explicitly distinguishes null projection metadata (not aware of projections) from an empty list (projection-aware, but has no projections), while the current condition still treats null as absence. A legacy projection-bearing segment can therefore take the no-download path and lose its stored projections during rewrite; an upgrade note cannot prevent that. Please treat null as unknown under a compatibility boundary, or inspect/guard it, and add a regression test for this case.
Reviewed 4 of 4 changed files for this follow-up.
<!-- mergelens:review -->
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "clintropolis (via GitHub)" <gi...@apache.org>.
clintropolis commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4420671309
##########
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:
left this because it would be too expensive i think to lump segments created by people with projections between 31 and 34 (when the property was added to DataSegment) is far lower than the number of segments that predate projections completely. Projections was not documented until Druid 36 afaict, so i think this is ok? Added to javadoc to try to justify this reasoning.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: preserve existing projections if omitted from inline compaction spec (druid)
Posted by "FrankChen021 (via GitHub) via commits" <co...@druid.apache.org>.
FrankChen021 commented on code in PR #20684:
URL: https://github.com/apache/druid/pull/20684#discussion_r4429482138
##########
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:
Thanks for documenting the performance trade-off. This still leaves the original finding open: DataSegment.getProjections() == null means the segment is not projection-aware, not that its index has no projections, and the Javadoc here acknowledges that legacy segments may still contain them. For such a segment, anySegmentHasProjections() returns false; with the other schema fields specified, compaction skips the index and can silently rewrite it without its projections. Please either treat null as unknown (or establish a trusted version/metadata boundary before treating it as empty) and add a regression test for that legacy/null case. Until that invariant is enforced, this remains a data-loss path, so I’m keeping it as P1.
<!-- mergelens:review -->
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org