kfaraz commented on code in PR #17960:
URL: https://github.com/apache/druid/pull/17960#discussion_r2069908370
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/input/ParseExceptionUtils.java:
##########
@@ -47,14 +44,9 @@ public static String
generateReadableInputSourceNameFromMappedSegment(Segment se
"external input source: %s",
((ExternalSegment) segment).externalInputSource().toString()
);
- } else if (segment instanceof LookupSegment) {
- return StringUtils.format("lookup input source: %s",
segment.getId().getDataSource());
Review Comment:
Should we still return this string without the segment id?
##########
processing/src/test/java/org/apache/druid/segment/TestSegmentUtils.java:
##########
@@ -260,6 +261,26 @@ public void close()
}
}
+ /**
+ * A test segment that is backed by a {@link RowBasedSegment}. This is used
to test the {@link QueryableIndexSegment}.
+ */
+ public static class InMemoryFakeSegment extends QueryableIndexSegment
Review Comment:
```suggestion
public static class InMemoryTestSegment extends QueryableIndexSegment
```
##########
processing/src/test/java/org/apache/druid/segment/TestSegmentUtils.java:
##########
@@ -121,77 +121,78 @@ public Segment factorize(
}
}
- public static class SegmentForTesting extends QueryableIndexSegment
implements Segment
+ private static final QueryableIndex INDEX = new QueryableIndex()
Review Comment:
We should consider adding a `NoopQueryIndex` impl as this is being done in a
couple of places in tests.
##########
processing/src/main/java/org/apache/druid/query/lookup/LookupSegment.java:
##########
@@ -44,10 +43,9 @@ public class LookupSegment extends
RowBasedSegment<Map.Entry<String, String>>
.add(LookupColumnSelectorFactory.VALUE_COLUMN,
ColumnType.STRING)
.build();
- public LookupSegment(final String lookupName, final LookupExtractorFactory
lookupExtractorFactory)
+ public LookupSegment(final LookupExtractorFactory lookupExtractorFactory)
Review Comment:
We should probably keep the lookup name argument and use it in `asString`
method of this class.
##########
processing/src/test/java/org/apache/druid/query/scan/ScanQueryResultOrderingTest.java:
##########
@@ -87,52 +89,59 @@ public class ScanQueryResultOrderingTest extends
InitializedNullHandlingTest
.add(ID_COLUMN, ColumnType.LONG)
.build();
- private static final List<RowBasedSegment<Object[]>> SEGMENTS =
ImmutableList.of(
- new RowBasedSegment<>(
+ private static final SegmentDescriptor DEFAULT_DESCRIPTOR = new
SegmentDescriptor(Intervals.ETERNITY, "", 0);
+ private static final List<InMemoryFakeSegment> SEGMENTS = ImmutableList.of(
+ new InMemoryFakeSegment(
SegmentId.of(DATASOURCE, Intervals.of("2000-01-01/P1D"), "1", 0),
- Sequences.simple(
- ImmutableList.of(
- new Object[]{DateTimes.of("2000T01"), 101},
- new Object[]{DateTimes.of("2000T01"), 80},
- new Object[]{DateTimes.of("2000T01"), 232},
- new Object[]{DateTimes.of("2000T01"), 12},
- new Object[]{DateTimes.of("2000T02"), 808},
- new Object[]{DateTimes.of("2000T02"), 411},
- new Object[]{DateTimes.of("2000T02"), 383},
- new Object[]{DateTimes.of("2000T05"), 22}
- )
- ),
- ROW_ADAPTER,
- ROW_SIGNATURE
+ new RowBasedSegment<>(
Review Comment:
The construction of the `RowBasedSegment` can be done inside the constructor
of `InMemoryFakeSegment` itself. That would help clean up this diff.
##########
processing/src/test/java/org/apache/druid/segment/TestSegmentUtils.java:
##########
@@ -260,6 +261,26 @@ public void close()
}
}
+ /**
+ * A test segment that is backed by a {@link RowBasedSegment}. This is used
to test the {@link QueryableIndexSegment}.
+ */
+ public static class InMemoryFakeSegment extends QueryableIndexSegment
+ {
+ private final RowBasedSegment<?> segment;
+
+ public InMemoryFakeSegment(SegmentId segmentId, RowBasedSegment<?> segment)
Review Comment:
Maybe accept a `Sequence` here instead and construct a `RowBasedSegment`
from it.
##########
processing/src/main/java/org/apache/druid/segment/Segment.java:
##########
@@ -38,6 +38,10 @@
@PublicApi
public interface Segment extends Closeable
{
+ /**
+ * Returns a {@link SegmentId} for a segment, if it's backed by a real
table, otherwise returns null.
Review Comment:
```suggestion
* Returns the {@link SegmentId} of this segment, if it is backed by a
real table, otherwise returns null.
```
##########
processing/src/main/java/org/apache/druid/segment/ReferenceCountingSegment.java:
##########
@@ -48,10 +48,11 @@ public class ReferenceCountingSegment extends
ReferenceCountingCloseableObject<S
public static ReferenceCountingSegment wrapRootGenerationSegment(Segment
baseSegment)
{
+ int partitionNum = baseSegment.getId() == null ? 0 :
baseSegment.getId().getPartitionNum();
return new ReferenceCountingSegment(
Preconditions.checkNotNull(baseSegment, "baseSegment"),
- baseSegment.getId().getPartitionNum(),
- (baseSegment.getId().getPartitionNum() + 1),
+ partitionNum,
+ (partitionNum + 1),
Review Comment:
Nit:
```suggestion
partitionNum + 1,
```
--
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]
