Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24504 )
Change subject: IMPALA-15101: Add UUID primitive type to Impala ...................................................................... Patch Set 4: (10 comments) Thanks for working on this, did a first pass of review. http://gerrit.cloudera.org:8080/#/c/24504/4/be/src/exprs/literal.cc File be/src/exprs/literal.cc: http://gerrit.cloudera.org:8080/#/c/24504/4/be/src/exprs/literal.cc@120 PS4, Line 120: if (str.size() == UUID_BYTE_LEN) { I don't think we need this branch, users won't really be able to write valid 16 bytes UUIDs as string literals. We should always expect the canonical string representation. http://gerrit.cloudera.org:8080/#/c/24504/4/be/src/exprs/literal.cc@124 PS4, Line 124: DCHECK DCHECK means ParseCanonicalUuidStringToBytes() is only invoked in debug builds. http://gerrit.cloudera.org:8080/#/c/24504/4/be/src/util/uuid-util.h File be/src/util/uuid-util.h: http://gerrit.cloudera.org:8080/#/c/24504/4/be/src/util/uuid-util.h@38 PS4, Line 38: inline void UuidBytesToString(const uint8_t* bytes, char* out) { Could you add backend tests for these? http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/analysis/InPredicate.java File fe/src/main/java/org/apache/impala/analysis/InPredicate.java: http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/analysis/InPredicate.java@60 PS4, Line 60: t.isUuid() Maybe we should add this separately when we can add tests as well. http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java File fe/src/main/java/org/apache/impala/analysis/LikePredicate.java: http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java@129 PS4, Line 129: for (int i = 0; i < 2; ++i) { : if (getChild(i).getType().isUuid()) { : uncheckedCastChild(Type.STRING, i); : } : } IN, LIKE, could be added in a follow-up ticket where we can test them http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/catalog/ScalarType.java File fe/src/main/java/org/apache/impala/catalog/ScalarType.java: http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/catalog/ScalarType.java@485 PS4, Line 485: UUID columns implicitly cast to STRING for string functions We could add implicit casts later, I'm also not sure if implicit upper(UUID), substr(UUId,...) makes sense. Probably it's better to require an explicit CAST, so it is clear that the string functions operate on the canonical representation. http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/catalog/Type.java File fe/src/main/java/org/apache/impala/catalog/Type.java: http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/catalog/Type.java@222 PS4, Line 222: isScalarType(PrimitiveType.UUID) What if we don't add UUID here for now? I think it makes sense to allow explicit UUID -> STRING casts where we would get the canonical string representation. But allowing UUID everywhere where we expect STRING seems dangerous. http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/catalog/Type.java@591 PS4, Line 591: 36 This should be 16. http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/catalog/UuidCompatibility.java File fe/src/main/java/org/apache/impala/catalog/UuidCompatibility.java: http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/catalog/UuidCompatibility.java@35 PS4, Line 35: UUID columns implicitly cast to STRING for string : // functions At first explicit casting is more secure IMO. And I think you'll need to add real CastStringToUuid / CastUuidToString BE functions and register them explicitly (don't rely on the identity CastToStringVal) as it won't create valid UUIDs. http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java: http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@3371 PS4, Line 3371: AnalyzesOk("create table functional.ice_uuid_complex (id INT, " + : "uuid_array ARRAY<UUID>, uuid_map MAP<UUID, STRING>) stored by iceberg Can you add the same test for a non-Iceberg table? -- To view, visit http://gerrit.cloudera.org:8080/24504 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iefc73aefe73b6144c929ec37b0cb333007cf8bfe Gerrit-Change-Number: 24504 Gerrit-PatchSet: 4 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 21 Jul 2026 17:22:09 +0000 Gerrit-HasComments: Yes
