Arnab Karmakar has posted comments on this change. ( http://gerrit.cloudera.org:8080/24504 )
Change subject: IMPALA-15101: Add Iceberg UUID primitive type ...................................................................... Patch Set 5: (10 comments) As instructed, Ive tried splitting the patch into smaller parts. This first patch includes the minimal necessary changes for adding uuid type. The rest of the query features will be shipped as part of the follow up patches. 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: int len = node.decimal_literal.val > I don't think we need this branch, users won't really be able to write vali Done http://gerrit.cloudera.org:8080/#/c/24504/4/be/src/exprs/literal.cc@124 PS4, Line 124: case 4 > DCHECK means ParseCanonicalUuidStringToBytes() is only invoked in debug bui Done 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? Done 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: > Maybe we should add this separately when we can add tests as well. Done. Split the change into smaller parts and we can add the support later on. 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: if (!isLikeableType(getChild(0).getType())) { : throw new AnalysisException( : "left operand of " + op_.toString() + " must be of type STRING: " + toSql()); : } : i > IN, LIKE, could be added in a follow-up ticket where we can test them Done. Split the change into smaller parts and we can add the support later on. 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: t2.type_.ordinal() ? t1.type_ : t2.type_); > We could add implicit casts later, I'm also not sure if implicit upper(UUID Done. Split the change into smaller parts and we can add CAST support later on. 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: boolean isScalarType() { return > What if we don't add UUID here for now? I think it makes sense to allow exp Done. Split the change into smaller parts and we can add CAST support later on. http://gerrit.cloudera.org:8080/#/c/24504/4/fe/src/main/java/org/apache/impala/catalog/Type.java@591 PS4, Line 591: e > This should be 16. I feel getColumnSize() is JDBC COLUMN_SIZE: the maximum display width in characters. Shouldnt that be 36 while slot size is returned by PrimitiveType.getSlotSize()? 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: : > At first explicit casting is more secure IMO. And I think you'll need to ad Done. CastToUuid/CastUuidToString functions were added in the second patch, but now Ive split the change into smaller parts and we can add CAST support later on. Only added a minimal UuidCompatibility that marks all UUID cross-type pairs as INVALID_TYPE. 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: // Tables with sort columns : AnalyzesOk("create table functional.new_table (i int, j int) sort by (i)") > Can you add the same test for a non-Iceberg table? Done -- 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: 5 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: Thu, 23 Jul 2026 06:34:40 +0000 Gerrit-HasComments: Yes
