imply-cheddar commented on code in PR #13268:
URL: https://github.com/apache/druid/pull/13268#discussion_r1034225520
##########
processing/src/main/java/org/apache/druid/query/UnnestDataSource.java:
##########
@@ -139,20 +135,27 @@ public Function<SegmentReference, SegmentReference>
createSegmentMapFunction(
AtomicLong cpuTimeAccumulator
)
{
+ final Function<SegmentReference, SegmentReference> segmentMapFn =
base.createSegmentMapFunction(
+ query,
+ cpuTimeAccumulator
+ );
return JvmUtils.safeAccumulateThreadCpuTime(
cpuTimeAccumulator,
() -> {
if (column == null) {
- return Function.identity();
+ return segmentMapFn;
} else if (column.isEmpty()) {
- return Function.identity();
+ return segmentMapFn;
} else {
- return baseSegment ->
- new UnnestSegmentReference(
- baseSegment,
- column,
- outputName,
- allowList
+ return
+ segmentMapFn.andThen(
Review Comment:
This is a style thing, but this sort of fluent style tends to produce
hard-to-read stack traces if there are any errors. It creates stack traces
with lines from the `Function` class rather than from `UnnestDataSource`.
Generally speaking, only use a fluent style when the fluency doesn't go outside
of the current scope of the code. If you are returning an object that is going
to be used by someone else, create a closure.
##########
processing/src/main/java/org/apache/druid/segment/ColumnarValueUnnestCursor.java:
##########
@@ -68,7 +70,10 @@ public ColumnSelectorFactory getColumnSelectorFactory()
@Override
public DimensionSelector makeDimensionSelector(DimensionSpec
dimensionSpec)
{
- return baseColumSelectorFactory.makeDimensionSelector(dimensionSpec);
+ if (!outputName.equals(dimensionSpec.getDimension())) {
+ return baseColumSelectorFactory.makeDimensionSelector(dimensionSpec);
+ }
+ return
baseColumSelectorFactory.makeDimensionSelector(DefaultDimensionSpec.of(columnName));
Review Comment:
I'm perhaps missing something, but this seems to be just delegating to the
base and returning without attempting to do any unnesting?
You likely haven't run into this because you are doing a validation ahead of
time for what the column can be. As such, the correct answer here might be to
throw an UnsupportedOperatorException instead as you are expecting the user to
be calling the ColumnValueSelector option instead.
--
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]
