xiangfu0 commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3561555643
##########
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;
+ }
+ if (!canonical.equals(uuidStr)) {
+ throw new IllegalArgumentException(
+ "Non-canonical UUID primary key value '" + uuidStr + "' in
upsert/dedup column '" + column + "'. "
+ + "Expected canonical lowercase RFC 4122 form: '" + canonical +
"'. "
+ + "UUID primary keys must be in canonical lowercase form to
ensure consistent Kafka partition "
+ + "routing. Non-canonical values cause silent dedup failures in
multi-partition realtime tables.");
+ }
+ }
Review Comment:
Fixed — `validateCanonicalUuidPrimaryKey` now throws when `UUID.fromString`
fails instead of returning, so no-dash 32-char hex (decodable by
`UuidUtils.toBytes`) and whitespace-padded values (trimmed by the type
conversion) are rejected. Only the exact canonical lowercase RFC-4122 string is
accepted.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
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:
Added regression cases for no-dash 32-char hex and whitespace-padded UUID
primary keys (both now expected to be rejected), alongside the existing
case-difference check.
_🤖 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]
