FrankChen021 commented on code in PR #19702:
URL: https://github.com/apache/druid/pull/19702#discussion_r3607330337
##########
processing/src/main/java/org/apache/druid/segment/projections/ClusteringColumnSelectorFactory.java:
##########
@@ -177,14 +185,54 @@ public ColumnCapabilities getColumnCapabilities(String
column)
@Override
public RowIdSupplier getRowIdSupplier()
{
- return delegate.getRowIdSupplier();
+ // A delegate may not support row-id caching; mirror that so callers skip
caching (null)
+ return delegate.getRowIdSupplier() == null ? null : rowIdSupplier;
+ }
+
+ /**
+ * The current row id, remapped to be unique across the whole concatenation
(see {@link #rowIdBase}). Records the
+ * delegate's raw row id as this group's running high-water mark for the
next transition.
+ */
+ long currentRowId()
+ {
+ final RowIdSupplier supplier = delegate.getRowIdSupplier();
+ if (supplier == null) {
+ // getRowIdSupplier() only hands out this wrapper when the delegate
supports row ids; clustered groups are
+ // homogeneous, so a null here would mean a group mid-cursor stopped
supporting them.
+ throw DruidException.defensive("delegate row id supplier became null
across a cluster-group transition");
+ }
+ final long rowId = supplier.getRowId();
+ lastDelegateRowId = rowId;
Review Comment:
[P1] Preserve the row-ID high-water mark
`lastDelegateRowId` is overwritten by the latest observation, but
`RowIdSupplier` explicitly permits non-monotonic IDs, and clustered cursors can
produce them through descending offsets or realtime row-index ordering. For
example, one cached selector may retain remapped ID 5, another selector may
later observe raw ID 1, and after transition raw ID 3 maps back to 5 because
the base advances only to 2. The first selector then reuses stale data from the
previous cluster group. Track a true maximum of observed nonnegative IDs or
issue wrapper-owned IDs, and cover descending/non-monotonic delegates.
##########
processing/src/main/java/org/apache/druid/segment/projections/ClusteringVectorColumnSelectorFactory.java:
##########
@@ -117,10 +125,35 @@ Object currentValue(int idx)
return clusteringValues[idx];
}
+ /**
+ * Current-vector size of the group the factory currently delegates to. Read
live so that a
+ * {@link DelegatingReadableVectorInspector} handed out once still reports
the active group's size after a
+ * {@link #setDelegate} transition.
+ */
+ int currentVectorSize()
+ {
+ return delegate.getReadableVectorInspector().getCurrentVectorSize();
+ }
+
+ /**
+ * The current vector id, remapped to be unique across the whole
concatenation. Returns
+ * {@link ReadableVectorInspector#NULL_ID} unchanged when the delegate has
no stable id, so callers still know
+ * caching is unsafe. Records the delegate's raw id as this group's running
high-water mark for the next transition.
+ */
+ int currentVectorId()
+ {
+ final int id = delegate.getReadableVectorInspector().getId();
+ if (id == ReadableVectorInspector.NULL_ID) {
+ return ReadableVectorInspector.NULL_ID;
+ }
+ lastDelegateId = id;
Review Comment:
[P2] Do not assume vector IDs increase
`ReadableVectorInspector` promises unique IDs but does not require monotonic
ordering. Overwriting `lastDelegateId` means a delegate sequence such as 5 then
1 advances the next group's base to only 2; raw ID 3 then collides with the
previously issued remapped ID 5, allowing a cached vector consumer to reuse
stale data. Use a collision-free wrapper-owned sequence or retain a true
high-water mark, with a non-monotonic inspector test.
--
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]
