Copilot commented on code in PR #19118:
URL: https://github.com/apache/pinot/pull/19118#discussion_r3686907709
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/segment/readers/PinotSegmentRecordReader.java:
##########
@@ -245,6 +263,14 @@ public void getRecord(int docId, GenericRow buffer) {
for (Map.Entry<String, PinotSegmentColumnReader> entry :
_columnReaderMap.entrySet()) {
String column = entry.getKey();
PinotSegmentColumnReader columnReader = entry.getValue();
+ if (column.equals(DEFAULT_UPSERT_COMPARISON_TIME_COLUMN)) {
+ Object comparisonTime = columnReader.isNull(docId) ? null :
columnReader.getValue(docId);
+ if (comparisonTime == null || ((Number) comparisonTime).longValue() ==
Long.MIN_VALUE) {
+ comparisonTime =
_indexSegment.getSegmentMetadata().getIndexCreationTime();
+ }
+ buffer.putValue(column, comparisonTime);
+ continue;
+ }
Review Comment:
This special-case reads the comparison-time value via `getValue()` (Object)
and then casts to `Number`, which introduces avoidable per-row
boxing/allocation when scanning segments (e.g., RefreshSegmentTask /
record-reader usage). Since this column is always LONG, using `getLong()` keeps
the fast-path primitive and simplifies the null/unset handling.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/upsert/BasePartitionUpsertMetadataManager.java:
##########
@@ -311,6 +313,9 @@ public void addSegment(ImmutableSegment segment) {
/// Creates a RecordInfoReader for the given segment. When comparison
columns are configured, reads comparison values
/// from the columns. When comparison columns are empty, uses segment
creation time as the comparison value.
protected UpsertUtils.RecordInfoReader createRecordInfoReader(IndexSegment
segment) {
+ if (usesImplicitComparisonTime()) {
+ return new UpsertUtils.RecordInfoReader(segment, _primaryKeyColumns,
_deleteRecordColumn);
+ }
Review Comment:
In implicit OFFLINE upsert mode, this always uses
`UpsertComparisonTimeColumnReader`. For legacy segments that omit the physical
`_upsert_comparison_ts` column, `getComparisonValue()` is invoked for every doc
and will box the fallback comparison time each time, which can create a large
amount of garbage while building upsert metadata. You can avoid the per-doc
boxing/allocation by using the existing constant-comparison reader path when
the segment doesn’t have a physical data source for the column.
--
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]
