Vamsi-klu commented on PR #18733: URL: https://github.com/apache/pinot/pull/18733#issuecomment-4912316800
Traced both branches. The out-of-order fix in doAddRecord is real and well-scoped in intent: versus today it only rescues a record whose value is < the stored one when that stored entry is out-of-TTL (the exact rejection this fixes). One correction though - it is not a clean no-op on the >= path. The expired branch runs whenever the entry is out-of-TTL, regardless of comparison order, so when new >= current AND expired it now handles the record instead of the normal >= branch. In the cross-segment isTableTypeInconsistentDuringConsumption() case that means it skips the _previousKeyToRecordLocationMap.put(primaryKey, currentRecordLocation) bookkeeping the >= branch does (which revertAndRemoveSegment relies on). That is exactly Copilot's L390 point and worth addressing rather than dismissing, since the expired branch subsumes the happy path too, not just the buggy one. Parts that do check out: the leading `_metadataTTL > 0 &&` is load-bearing, not redundant with isOutOfMetadataTTL's own guard - it short-circuits the `(Number) getComparisonValue()` cast in the argument, so non-TTL tables with non-numeric comparison columns cannot ClassCastException. And skipping the ForConsistentDeletes subclass is fine: TableConfigUtils forbids metadataTTL alongside enableDeletedKeysCompactionConsistency, so the branch is unreachable there. Coverage gap: the new test only drives doAddRecord. The doUpdateRecord guard has no test and is the riskier half - updateRecord() runs before addRecord() bumps _largestSeenComparisonValue (doAddRecord updates the watermark at the top; doUpdateRecord does not), so its expiry check can read a watermark one record behind doAddRecord's and still merge a stale row. A partial-upsert record against an expired previous row would close the gap and settle whether that branch actually skips the merge. Also worth a rebase to re-trigger CI - the green run is fairly far behind master. -- 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]
