Copilot commented on code in PR #18733:
URL: https://github.com/apache/pinot/pull/18733#discussion_r3538972656
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -423,7 +436,9 @@ protected GenericRow doUpdateRecord(GenericRow record,
RecordInfo recordInfo) {
// - New record is not out-of-order
// - Previous record is not deleted
if (!recordInfo.isDeleteRecord()
- &&
recordInfo.getComparisonValue().compareTo(recordLocation.getComparisonValue())
>= 0) {
+ &&
recordInfo.getComparisonValue().compareTo(recordLocation.getComparisonValue())
>= 0
+ && !(_metadataTTL > 0 && isOutOfMetadataTTL(
+ ((Number)
recordLocation.getComparisonValue()).doubleValue()))) {
Review Comment:
The TTL guard in doUpdateRecord() checks expiration via
isOutOfMetadataTTL(recordLocationValue), which uses
_largestSeenComparisonValue. But updateRecord() is invoked before addRecord()
(see MutableSegmentImpl#index() flow), so _largestSeenComparisonValue may not
yet include the current record’s comparison value. This can still merge a stale
previous row when the current record advances the watermark enough to expire it
(exact scenario the PR is trying to prevent).
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/ConcurrentMapPartitionUpsertMetadataManager.java:
##########
@@ -378,6 +378,19 @@ protected boolean doAddRecord(MutableSegment segment,
RecordInfo recordInfo) {
if (currentRecordLocation != null) {
// Existing primary key
IndexSegment currentSegment = currentRecordLocation.getSegment();
+
+ // If existing metadata is expired per TTL, treat as new key
+ if (_metadataTTL > 0 && isOutOfMetadataTTL(
+ ((Number)
currentRecordLocation.getComparisonValue()).doubleValue())) {
+ int currentDocId = currentRecordLocation.getDocId();
+ if (segment == currentSegment) {
+ replaceDocId(segment, validDocIds, queryableDocIds,
currentDocId, newDocId, recordInfo);
+ } else {
+ replaceDocId(segment, validDocIds, queryableDocIds,
currentSegment, currentDocId, newDocId, recordInfo);
+ }
Review Comment:
In the new "expired metadata" branch, when replacing a location from a
different (often immutable) segment, the code doesn’t preserve the previous
RecordLocation in _previousKeyToRecordLocationMap. The normal replacement path
does this when isTableTypeInconsistentDuringConsumption() is enabled so
revertAndRemoveSegment() can restore keys if the consuming segment is rolled
back/removed. The TTL replacement should mirror that bookkeeping for the same
inconsistency mode.
--
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]
