xiangfu0 commented on code in PR #20068:
URL: https://github.com/apache/pinot/pull/20068#discussion_r4401628384
##########
pinot-core/src/main/java/org/apache/pinot/core/query/prefetch/FetchPlanner.java:
##########
@@ -27,21 +28,27 @@
/// read from some slow storage backend, to reduce the query latency. Although
the plan methods take in an indexSegment
/// object, it should mainly access the segment metadata like what types of
index it has, as the index data may not be
/// available, but leaving this to the implementations to check and handle per
their own need.
+///
+/// A planner can return null for a segment that needs no fetch, e.g. one
whose data is all local, so its fetch calls
+/// would be no-ops. The caller then skips the fetch lifecycle (prefetch,
acquire, release) for that segment entirely.
+/// A non-null context, even an empty one, keeps the lifecycle.
/// TODO: this interface and the dependent classes like QueryContext should be
moved to query-spi pkg, yet to be added.
public interface FetchPlanner {
/// Plan what index data to prefetch to help prune the segment before
processing it. For example, one can fetch bloom
/// filter to prune the segment based on the column values in query
predicates like IN or EQ.
///
/// @param indexSegment segment to be pruned.
/// @param queryContext context extracted from the query.
- /// @return context to guide data prefetching.
+ /// @return context to guide data prefetching, or null if the segment needs
no fetch.
+ @Nullable
Review Comment:
**MINOR:** The real behavior change is this new nullable contract:
out-of-tree callers of `planFetchFor*` now have to null-check. The PR
description only talks about `requiresFetch()`. Please update it so it
describes the null-return contract (and maybe add a release note for plugin
authors).
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/IndexSegment.java:
##########
@@ -122,6 +122,14 @@ default boolean hasDeletedDocIds() {
/// values from the same segment.
Object getValue(int docId, String column);
+ /// Returns whether this segment serves buffers through [#prefetch],
[#acquire] and [#release]. When it returns false,
+ /// those calls are no-ops, so a fetch planner can skip planning a
[FetchContext] for the segment, letting query
+ /// execution skip the fetch lifecycle altogether. Segments that override
the fetch calls must return true. The value
+ /// must not change over the lifetime of the segment.
+ default boolean requiresFetch() {
Review Comment:
**MAJOR:** Nothing in this PR calls `requiresFetch()`. `DefaultFetchPlanner`
ignores it, no test covers it, and the skipping actually comes from the planner
returning null. It also defaults to `false` (no fetch needed) here and in
`SegmentDirectory`. So any existing out-of-tree directory or segment that
overrides `prefetch`/`acquire`/`release` will report that it needs no fetch
until someone updates it, and a planner that trusts the flag would skip acquire
for remote data. Could we either default to `true` (with
`SegmentLocalFSDirectory` overriding it to `false`) or leave the flag out of
this PR until a planner uses it and has tests?
--
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]
