Jackie-Jiang commented on code in PR #10915:
URL: https://github.com/apache/pinot/pull/10915#discussion_r1267660646
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -274,6 +309,13 @@ protected void
addSegmentWithoutUpsert(ImmutableSegmentImpl segment, ThreadSafeM
addOrReplaceSegment(segment, validDocIds, queryableDocIds,
recordInfoIterator, null, null);
}
+ protected void enableSegmentWithSnapshot(ImmutableSegmentImpl segment,
ThreadSafeMutableRoaringBitmap validDocIds,
+ @Nullable ThreadSafeMutableRoaringBitmap queryableDocIds) {
+ MutableRoaringBitmap validDocIdsSnapshot =
segment.loadValidDocIdsFromSnapshot();
+ validDocIdsSnapshot.forEach((int docId) -> validDocIds.add(docId));
Review Comment:
Add a check `if (validDocIdsSnapshot != null)`, and consider how to handle
the case when snapshot doesn't exist. In normal case snapshot should exist, but
we don't want NPE here
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -205,6 +206,18 @@ private static void removeDocId(IndexSegment segment, int
docId) {
@Override
protected void removeSegment(IndexSegment segment, MutableRoaringBitmap
validDocIds) {
assert !validDocIds.isEmpty();
+
+ // Skip removing segments that has segment EndTime in the comparison cols
earlier than (largestSeenTimestamp - TTL).
Review Comment:
This part should be moved to the
`BasePartitionUpsertMetadataManager.removeSegment(ImmutableSegment segment)`
before locking the snapshot read lock
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -253,6 +273,21 @@ public void addSegment(ImmutableSegmentImpl segment,
@Nullable ThreadSafeMutable
if (queryableDocIds == null && _deleteRecordColumn != null) {
queryableDocIds = new ThreadSafeMutableRoaringBitmap();
}
+ // Skip adding segments that has segment EndTime in the comparison cols
earlier than (largestSeenTimestamp - TTL).
Review Comment:
This part should be moved to the
`BasePartitionUpsertMetadataManager.addSegment(ImmutableSegment segment)`
before locking the snapshot read lock. Here it is already too late and we
already performed the expensive work
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/utils/TableConfigUtils.java:
##########
@@ -595,6 +595,39 @@ &&
isRoutingStrategyAllowedForUpsert(tableConfig.getRoutingConfig()),
}
}
validateAggregateMetricsForUpsertConfig(tableConfig);
+ validateTTLForUpsertConfig(tableConfig, schema);
+ }
+
+ /**
+ * Validates whether the comparison columns is compatible with Upsert TTL
feature.
+ * Validation fails when one of the comparison columns is not a numeric
value.
+ */
+ @VisibleForTesting
+ static void validateTTLForUpsertConfig(TableConfig tableConfig, Schema
schema) {
+ if (tableConfig.getUpsertMode() == UpsertConfig.Mode.NONE
+ || tableConfig.getUpsertConfig().getUpsertTTLInComparisonTimeUnit() ==
0) {
+ return;
+ }
+
+ // comparison columns should hold timestamp values in numeric values
+ List<String> comparisonColumns =
tableConfig.getUpsertConfig().getComparisonColumns();
+ if (comparisonColumns != null && !comparisonColumns.isEmpty()) {
+
+ // currently we only support 1 comparison column since we need to fetch
endTime in comparisonValue time unit from
+ // columnMetadata. If we have multiple comparison columns, we can only
use the first comparison column as filter.
+ Preconditions.checkState(comparisonColumns.size() <= 1,
Review Comment:
You already have that check on line 614
--
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]
