xiangfu0 commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3525496957
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/indexsegment/mutable/MutableSegmentImpl.java:
##########
@@ -780,16 +774,37 @@ private DedupRecordInfo getDedupRecordInfo(GenericRow
row) {
}
private RecordInfo getRecordInfo(GenericRow row, int docId) {
- PrimaryKey primaryKey = row.getPrimaryKey(_schema.getPrimaryKeyColumns());
+ PrimaryKey primaryKey = getPrimaryKey(row);
Comparable comparisonValue = getComparisonValue(row);
boolean deleteRecord = _deleteRecordColumn != null &&
BooleanUtils.toBoolean(row.getValue(_deleteRecordColumn));
return new RecordInfo(primaryKey, docId, comparisonValue, deleteRecord);
}
+ private PrimaryKey getPrimaryKey(GenericRow row) {
+ List<String> primaryKeyColumns = _schema.getPrimaryKeyColumns();
+ int numPrimaryKeyColumns = primaryKeyColumns.size();
+ Object[] values = new Object[numPrimaryKeyColumns];
+ for (int i = 0; i < numPrimaryKeyColumns; i++) {
+ String primaryKeyColumn = primaryKeyColumns.get(i);
+ Object value = row.getValue(primaryKeyColumn);
+ DataType dataType =
_schema.getFieldSpecFor(primaryKeyColumn).getDataType();
+ values[i] = normalizePrimaryKeyValue(value, dataType);
+ }
Review Comment:
Fixed — `normalizePrimaryKeyValue` now throws `IllegalArgumentException("Got
null value for UUID primary key column: <column>")` via a
`Preconditions.checkArgument` null guard before calling `UuidUtils.toBytes`
(which intentionally has no null checks on the hot path).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/index/creator/BloomFilterCreator.java:
##########
@@ -44,12 +47,25 @@ default void add(Object[] values, @Nullable int[] dictIds) {
for (Object value : values) {
add(BytesUtils.toHexString((byte[]) value));
}
+ } else if (getDataType() == FieldSpec.DataType.UUID) {
+ for (Object value : values) {
+ add(uuidToCanonicalString(value));
+ }
} else {
for (Object value : values) {
add(value.toString());
}
}
}
+
+ /// Renders a UUID value (typically a 16-byte big-endian {@code byte[]} from
segment ingest) as its canonical
+ /// lowercase RFC 4122 string. Avoids the defensive byte[] copy that {@code
UuidUtils.toBytes(Object)} would
+ /// perform on the {@code byte[]} branch.
Review Comment:
Fixed — the Javadoc now states that neither path copies the buffer
(`UuidUtils.toBytes(byte[])` validates the width and returns it as-is); the
`byte[]` fast path only skips the `toBytes(Object)` type dispatch. The stale
"defensive copy" wording predated the #18869 review change that removed the
copy.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
