xiangfu0 commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3599570171
##########
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:
Fixed. `validateCanonicalUuidPrimaryKey` now rejects any value that isn't
exactly the canonical dashed-lowercase form — the parse-failure path throws
instead of returning, so the dash-less 32-hex form (accepted by
`UuidUtils.toBytes` via the `BytesUtils.toBytes` hex fallback) and
whitespace-padded values are now rejected rather than silently normalized to
the same bytes.
_🤖 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 coverage: the test now also asserts that the dash-less
32-hex (`550e8400e29b41d4a716446655440000`) and whitespace-padded UUID PK forms
are rejected in an upsert table, alongside the existing uppercase case.
_🤖 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]
