Copilot commented on code in PR #18870:
URL: https://github.com/apache/pinot/pull/18870#discussion_r3561504711
##########
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 canonical-PK validation is meant to prevent Kafka partition-routing
inconsistencies, which also applies to UUID strings that are accepted by UUID
conversion but are not canonical RFC-4122 text (e.g. no-dash hex, or values
with surrounding whitespace). This test currently only covers case differences;
adding a regression check for no-dash/whitespace inputs would prevent future
regressions (especially since `UuidUtils.toBytes(String)` accepts hex-encoded
16-byte strings).
##########
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:
`validateCanonicalUuidPrimaryKey()` returns on `UUID.fromString(...)` parse
failure, which unintentionally allows non-canonical UUID *strings* like no-dash
hex (decoded by `UuidUtils.toBytes(String)` via `BytesUtils.toBytes`) or
strings with surrounding whitespace to be accepted for upsert/dedup UUID PKs.
Those variants will hash/route differently at the Kafka producer, causing the
same silent cross-partition dedup failure this check is meant to prevent.
Instead, any UUID PK string that is not parseable by `UUID.fromString` should
be rejected here (and only the exact lowercase canonical form should be
accepted).
--
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]
