Copilot commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3599535637
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/recordtransformer/DataTypeTransformer.java:
##########
@@ -107,4 +146,36 @@ public void transform(GenericRow record) {
}
}
}
+
+ /**
+ * Validates that a UUID primary key string value is in canonical lowercase
RFC 4122 form.
+ *
+ * <p>UUID primary keys in upsert/dedup realtime tables must be canonical
because Kafka partition routing
+ * is determined by the raw string value that the producer sends. If the
producer sends the same logical
+ * UUID with different casing (e.g. uppercase vs lowercase), Kafka will hash
the strings differently
+ * and the messages may land on different partitions. Pinot then normalises
them to the same bytes
+ * inside each consuming segment, so within-segment dedup works, but
cross-partition dedup never
+ * fires - producing duplicate or stale rows silently.
+ *
+ * <p>By rejecting non-canonical UUIDs here (before bytes conversion loses
the original string),
+ * we ensure that any producer-side casing inconsistency surfaces as an
ingestion error rather
+ * than a silent correctness failure. Producers must canonicalise UUID
primary keys to lowercase
+ * before publishing to Kafka.
+ */
+ private static void validateCanonicalUuidPrimaryKey(String column, String
uuidStr) {
+ String canonical;
+ try {
+ canonical = UUID.fromString(uuidStr).toString();
+ } catch (IllegalArgumentException e) {
+ // Malformed UUID; let DataTypeTransformerUtils.transformValue produce
the standard error.
+ return;
+ }
Review Comment:
`validateCanonicalUuidPrimaryKey` returns on `UUID.fromString` parse
failure, which allows non-canonical UUID primary-key strings (e.g. 32-hex
no-dash form accepted by `UuidUtils.toBytes(String)` via `BytesUtils.toBytes`)
or whitespace-padded values to slip through in upsert/dedup tables. This
defeats the purpose of canonical-PK enforcement for stable Kafka partition
routing; malformed/unparseable UUID PK strings should be rejected here.
##########
pinot-segment-local/src/test/java/org/apache/pinot/segment/local/recordtransformer/DataTypeTransformerTest.java:
##########
@@ -201,4 +209,82 @@ public void testStandardize() {
}
assertEqualsNoOrder((Object[])
DataTypeTransformerUtils.standardize(COLUMN, values, false), expectedValues);
}
+
+ /**
+ * Verifies that non-canonical (uppercase) UUID strings in upsert/dedup
primary key columns are rejected,
+ * while canonical lowercase UUIDs are accepted, and non-primary-key UUID
columns are unaffected.
+ */
+ @Test
+ public void testUuidUpsertPrimaryKeyCanonicalValidation() {
+ String uuidCol = "uuidPk";
+ String nonPkUuidCol = "uuidOther";
+ String canonicalUuid = "550e8400-e29b-41d4-a716-446655440000";
+ String uppercaseUuid = "550E8400-E29B-41D4-A716-446655440000";
+
+ Schema schema = new Schema.SchemaBuilder()
+ .addSingleValueDimension(uuidCol, FieldSpec.DataType.UUID)
+ .addSingleValueDimension(nonPkUuidCol, FieldSpec.DataType.UUID)
+ .setPrimaryKeyColumns(List.of(uuidCol))
+ .build();
+ TableConfig upsertTableConfig = new TableConfigBuilder(TableType.REALTIME)
+ .setTableName("testUpsertUuid")
+ .setUpsertConfig(new UpsertConfig(UpsertConfig.Mode.FULL))
+ .build();
+
+ DataTypeTransformer upsertTransformer = new
DataTypeTransformer(upsertTableConfig, schema);
+
+ // Canonical lowercase UUID primary key: accepted
+ GenericRow canonicalRow = new GenericRow();
+ canonicalRow.putValue(uuidCol, canonicalUuid);
+ canonicalRow.putValue(nonPkUuidCol, canonicalUuid);
+ upsertTransformer.transform(canonicalRow); // must not throw
+
+ // Non-canonical uppercase UUID primary key: rejected
+ GenericRow uppercaseRow = new GenericRow();
+ uppercaseRow.putValue(uuidCol, uppercaseUuid);
+ uppercaseRow.putValue(nonPkUuidCol, canonicalUuid);
+ try {
+ upsertTransformer.transform(uppercaseRow);
+ fail("Expected RuntimeException for non-canonical UUID primary key in
upsert table");
+ } catch (RuntimeException e) {
+ // Expected: DataTypeTransformer wraps the IllegalArgumentException in a
RuntimeException
+ }
Review Comment:
The new canonical-PK validation test only covers case differences. Add
regression coverage for other non-canonical UUID primary-key strings that could
previously be accepted by UUID normalization (e.g. 32-hex no-dash form and
whitespace-padded values) so future changes don’t accidentally reintroduce
Kafka-partition routing inconsistencies for upsert/dedup.
--
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]
