clintropolis commented on code in PR #17943:
URL: https://github.com/apache/druid/pull/17943#discussion_r2060808174
##########
extensions-core/multi-stage-query/src/main/java/org/apache/druid/msq/dart/worker/DartDataSegmentProvider.java:
##########
@@ -95,7 +96,10 @@ public Supplier<ResourceHolder<CompleteSegment>>
fetchSegment(
final PhysicalSegmentInspector inspector =
segment.as(PhysicalSegmentInspector.class);
channelCounters.addFile(inspector != null ? inspector.getNumRows() :
0, 0);
});
- return new ReferenceCountingResourceHolder<>(new CompleteSegment(null,
segment), closer);
+ return new ReferenceCountingResourceHolder<>(new CompleteSegment(
+ null,
+ Objects.requireNonNull(segment.getBaseSegment())
+ ), closer);
Review Comment:
>Btw, a side note I have realized as part of thinking about this: in native,
we apply the segment map fn first and then acquire a segment reference; whereas
with Dart we acquire the reference first and then apply the segment map fn.
IMO, the way we do it in Dart makes more sense. If native worked the same way,
then segment map fns could use Segment rather than SegmentReference, and the
call to ReferenceCountingSegment.wrapRootGenerationSegment in
BaseLeafFrameProcessor wouldn't necessary. (There is no need to change this
right now, it's fine, I'm just making an observation.)
I think if we did this it is maybe confusing for `SegmentReference` to exist
at all (which is fair, it is kind of confusing), and instead should just be a
`ReferenceCountedObject` that doesn't implement `Segment` and we use to get a
`Segment`. This sounds like a pretty big change, though I guess would make
things less ambiguous of whether or not we are dealing with the "real"
`Segment` or the reference counting wrapper.
I do think that one nice thing of the way things are right now is that any
bugs caused by having a stale reference to a reference counting segment that
has actually been closed are maybe a bit clearer what is going on by using the
`SegmentReference` as a proxy for operating on `Segment` because it returns
null for all of the things if the backing segment has been
closed/unmapped/dropped instead of what i imagine to be a variety of different
errors if you had a stale reference to the real `Segment` object and tried to
do things with it.
--
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]
