gianm commented on code in PR #19726:
URL: https://github.com/apache/druid/pull/19726#discussion_r3636530093
##########
processing/src/main/java/org/apache/druid/segment/QueryableIndexCursorFactory.java:
##########
@@ -200,15 +201,49 @@ private CursorHolder makeSingleGroupClusteredCursorHolder(
);
}
+ // A single cluster group is physically sorted by its group ordering (the
segment ordering with the constant
+ // clustering prefix dropped). When the query wants __time ordering and
__time is the first non-clustering column,
+ // advertise that __time-first group ordering so the holder reports (and
honors, incl. descending) time ordering --
+ // matching the multi-group merge path, so the same query does not depend
on how many groups survive pruning.
+ // Otherwise advertise the full clustering-first segment ordering. Note
the clustering prefix is constant across
+ // this single group, so both orderings are truthful descriptions of the
exposed rows.
+ final ClusteredValueGroupsBaseTableSchema summary =
valueGroup.getSummary();
+ final List<OrderBy> ordering =
+ isGroupTimeOrderingRequested(spec, summary) ?
summary.getGroupOrdering() : summary.getOrdering();
+
// groupIndex exposes the group's clustering columns as constant columns,
no selector wrapper is needed
return new QueryableIndexCursorHolder(
groupIndex,
plan.rebuildCursorBuildSpec(spec, valueGroup),
QueryableIndexTimeBoundaryInspector.create(groupIndex),
- valueGroup.getSummary().getOrdering()
+ ordering
);
}
+ /**
+ * Whether the query requests {@code __time} ordering and each cluster group
is individually {@code __time}-sorted
+ * (i.e. {@code __time} is the first non-clustering column, so {@link
ClusteredValueGroupsBaseTableSchema#getGroupOrdering()}
+ * is {@code __time}-first). In that case a clustered read can serve a
globally {@code __time}-ordered cursor: the
+ * single-group path advertises the group's {@code __time}-first ordering
directly, and the multi-group path k-way
+ * merges the groups (see {@link #makeTimeMergedClusteredCursorHolder} /
{@link MergingClusterGroupCursor}).
+ */
+ private static boolean isGroupTimeOrderingRequested(CursorBuildSpec spec,
ClusteredValueGroupsBaseTableSchema summary)
Review Comment:
IMO `useTimeOrderedCursors` is a better name. The current name makes it
sound like it's all about was requested, not necessarily what's possible. (The
caller might request time ordering in the CursorBuildSpec, but it may not be
possible to satisfy the request.)
--
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]
