Vamsi-klu commented on PR #18859: URL: https://github.com/apache/pinot/pull/18859#issuecomment-4912317448
Read through the fix; it looks correct and well-scoped. Map.Entry.<String, LineageEntry>comparingByValue(comparingLong(getTimestamp)).thenComparing(comparingByKey()) is a total order (timestamp primary, unique map-key id as tiebreaker), so forEachOrdered into a LinkedHashMap makes toJsonObject() output deterministic. The explicit <String, LineageEntry> type witness is genuinely needed here, otherwise the key type is unconstrained and thenComparing(comparingByKey()) won't resolve. Nice that the test isn't tautological: it re-reads the persisted SegmentLineage and derives the expected order with the same (timestamp, then id) comparator, so the assertion is driven by the id tiebreaker rather than a hardcoded order. Worth noting the regression guard is probabilistic, not deterministic: it only catches a revert to timestamp-only sorting on runs where the two entries tie on the same millisecond and HashMap iteration order disagrees with id order. When the timestamps differ, both old and new sorts agree and the test passes either way. That's fine given the whole point is flaky sub-millisecond ties, just calling it out so no one assumes it hard-fails on any revert. Clean, low-risk fix. LGTM. -- 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]
