gammacomputer commented on issue #19270:
URL: https://github.com/apache/druid/issues/19270#issuecomment-4205120455
Hello Gianm,
Below is the fix proposed by the LLM, I hope it can help.
Also it found that FilteredSegment maybe has the same pattern, this is not
covered in this fix: _FilteredSegment.as() also returns null for
TimeBoundaryInspector. However, a filter could theoretically narrow the
effective time range, so delegating the inspector there is less straightforward
-- the delegate's boundaries would be an upper bound, not exact. This might
still be acceptable (an overestimate is better than no clipping at all), but
it's a separate discussion._
Root Cause
UnnestSegment.as(TimeBoundaryInspector.class) returns null. When
TimeseriesQueryRunnerFactory creates a runner (line 65 of
TimeseriesQueryRunnerFactory.java), it calls
segment.as(TimeBoundaryInspector.class)
to get the segment's time boundaries. Since UnnestSegment returns null for
this, CursorGranularizer.create() cannot clip the query interval. With a wide
interval (like ETERNITY, which is the default when no
WHERE __time filter is provided), granularity.getIterable() generates
billions of buckets for non-ALL granularities, and the engine spends all its
time iterating over empty buckets.
Fix from commit 8a8160157172e14a3052597e2cff4950d8ede245 (origin/main
currently)
The fix is a single addition to UnnestSegment.as() in
processing/src/main/java/org/apache/druid/segment/UnnestSegment.java:
```
diff --git
a/processing/src/main/java/org/apache/druid/segment/UnnestSegment.java
b/processing/src/main/java/org/apache/druid/segment/UnnestSegment.java
index a82e60ab8b..5ae214d30c 100644
--- a/processing/src/main/java/org/apache/druid/segment/UnnestSegment.java
+++ b/processing/src/main/java/org/apache/druid/segment/UnnestSegment.java
@@ -49,6 +49,9 @@ public class UnnestSegment extends WrappedSegment
return (T) new UnnestCursorFactory(delegate.as(CursorFactory.class),
unnestColumn, filter);
} else if (TopNOptimizationInspector.class.equals(clazz)) {
return (T) new SimpleTopNOptimizationInspector(filter == null);
+ } else if (TimeBoundaryInspector.class.equals(clazz)) {
+ // Unnest does not alter the __time column, so the delegate's time
boundaries are still valid.
+ return delegate.as(clazz);
}
return null;
}
```
This delegates TimeBoundaryInspector to the underlying segment. This is safe
because unnest does not modify the __time column -- it only expands array
values into additional rows that share the same
timestamp as the original row. With this change, CursorGranularizer clips
the interval to the actual data range, and the bucket iteration covers only the
relevant time span.
Test
Added UnnestTimeseriesQueryRunnerTest in
processing/src/test/java/org/apache/druid/query/timeseries/. The test runs a
timeseries query with DAY granularity over an unnest segment using the ETERNITY
interval.
It has a @Test(timeout = 10000) annotation so it fails in 10 seconds if the
fix is reverted, instead of hanging. The test verifies that 2 rows across 2
days produce correct per-day counts (3 and 2 unnested
values respectively).
Testing
Without the fix (fix temporarily reverted, test kept):
```
docker run --rm -v "$(pwd)":/src -v druid-m2-cache:/root/.m2 -w /src \
maven:3.9-eclipse-temurin-21 bash -c \
"git config --global --add safe.directory /src && \
mvn test -pl processing \
-Dtest='UnnestTimeseriesQueryRunnerTest' \
-DfailIfNoTests=false -B"
```
Result: Tests run: 1, Failures: 0, Errors: 1, Skipped: 0 -- BUILD FAILURE
(timeout after 10 seconds, confirming the bug)
With the fix (all unnest-related tests):
```
docker run --rm -v "$(pwd)":/src -v druid-m2-cache:/root/.m2 -w /src \
maven:3.9-eclipse-temurin-21 bash -c \
"git config --global --add safe.directory /src && \
mvn test -pl processing \
-Dtest='UnnestCursorFactoryTest,UnnestColumnValueSelectorCursorTest,UnnestGroupByQueryRunnerTest,UnnestScanQueryRunnerTest,UnnestTopNQueryRunnerTest,UnnestTimeseriesQueryRunnerTest'
\
-DfailIfNoTests=false -B"
```
Result: Tests run: 71, Failures: 0, Errors: 0, Skipped: 0 -- BUILD SUCCESS
Here is the test file at
`processing/src/test/java/org/apache/druid/query/timeseries/UnnestTimeseriesQueryRunnerTest.java`
```
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.apache.druid.query.timeseries;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import org.apache.druid.data.input.MapBasedInputRow;
import org.apache.druid.java.util.common.DateTimes;
import org.apache.druid.java.util.common.Intervals;
import org.apache.druid.java.util.common.granularity.Granularities;
import org.apache.druid.query.Druids;
import org.apache.druid.query.FinalizeResultsQueryRunner;
import org.apache.druid.query.QueryPlus;
import org.apache.druid.query.QueryRunner;
import org.apache.druid.query.QueryRunnerFactory;
import org.apache.druid.query.QueryRunnerTestHelper;
import org.apache.druid.query.Result;
import org.apache.druid.query.TableDataSource;
import org.apache.druid.query.UnnestDataSource;
import org.apache.druid.query.aggregation.CountAggregatorFactory;
import org.apache.druid.query.expression.TestExprMacroTable;
import org.apache.druid.segment.IncrementalIndexSegment;
import org.apache.druid.segment.Segment;
import org.apache.druid.segment.UnnestSegment;
import org.apache.druid.segment.column.ColumnType;
import org.apache.druid.segment.incremental.IncrementalIndex;
import org.apache.druid.segment.incremental.IncrementalIndexSchema;
import org.apache.druid.segment.incremental.OnheapIncrementalIndex;
import org.apache.druid.segment.virtual.ExpressionVirtualColumn;
import org.apache.druid.timeline.SegmentId;
import org.junit.Assert;
import org.junit.Test;
import java.util.Arrays;
import java.util.List;
/**
* Tests for timeseries queries over unnest datasources.
* Specifically verifies that timeseries queries with non-ALL granularity
* complete in reasonable time when the query interval is wide (e.g.,
ETERNITY),
* which requires proper TimeBoundaryInspector propagation through
UnnestSegment.
*/
public class UnnestTimeseriesQueryRunnerTest
{
@Test(timeout = 10000)
public void
testTimeseriesOverUnnestWithDayGranularityAndEternityInterval() throws Exception
{
final IncrementalIndex index = new OnheapIncrementalIndex.Builder()
.setIndexSchema(
new IncrementalIndexSchema.Builder()
.withMinTimestamp(DateTimes.of("2024-01-01T00:00:00Z").getMillis())
.build()
)
.setMaxRowCount(1000)
.build();
index.add(
new MapBasedInputRow(
DateTimes.of("2024-01-01T00:00:00Z").getMillis(),
ImmutableList.of("dim1"),
ImmutableMap.of("dim1", Arrays.asList("a", "b", "c"))
)
);
index.add(
new MapBasedInputRow(
DateTimes.of("2024-01-02T00:00:00Z").getMillis(),
ImmutableList.of("dim1"),
ImmutableMap.of("dim1", Arrays.asList("d", "e"))
)
);
final ExpressionVirtualColumn unnestColumn = new ExpressionVirtualColumn(
"unnested",
"\"dim1\"",
ColumnType.STRING,
TestExprMacroTable.INSTANCE
);
final Segment baseSegment = new IncrementalIndexSegment(index,
SegmentId.dummy("ds"));
final Segment unnestSegment = new UnnestSegment(baseSegment,
unnestColumn, null);
final QueryRunnerFactory factory = new TimeseriesQueryRunnerFactory(
new TimeseriesQueryQueryToolChest(),
new TimeseriesQueryEngine(),
QueryRunnerTestHelper.NOOP_QUERYWATCHER
);
final QueryRunner<Result<TimeseriesResultValue>> runner =
new
FinalizeResultsQueryRunner<>(factory.createRunner(unnestSegment),
factory.getToolchest());
// Use ETERNITY interval with DAY granularity. Without the fix
(UnnestSegment delegating
// TimeBoundaryInspector), CursorGranularizer cannot clip the interval,
causing it to
// iterate over billions of empty day-sized buckets and effectively hang.
TimeseriesQuery query = Druids.newTimeseriesQueryBuilder()
.dataSource(
UnnestDataSource.create(
new TableDataSource("ds"),
unnestColumn,
null
)
)
.granularity(Granularities.DAY)
.intervals(ImmutableList.of(Intervals.ETERNITY))
.aggregators(new
CountAggregatorFactory("rows"))
.context(ImmutableMap.of(TimeseriesQuery.SKIP_EMPTY_BUCKETS, true))
.build();
List<Result<TimeseriesResultValue>> results =
runner.run(QueryPlus.wrap(query)).toList();
Assert.assertEquals("result size", 2, results.size());
Assert.assertEquals("day 1 timestamp", DateTimes.of("2024-01-01"),
results.get(0).getTimestamp());
Assert.assertEquals("day 1 count", 3L, (long)
results.get(0).getValue().getLongMetric("rows"));
Assert.assertEquals("day 2 timestamp", DateTimes.of("2024-01-02"),
results.get(1).getTimestamp());
Assert.assertEquals("day 2 count", 2L, (long)
results.get(1).getValue().getLongMetric("rows"));
}
}
```
--
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]
